Performance improvements to TagSerializer#155
Conversation
| end | ||
|
|
||
| tags = if @global_tags_formatted | ||
| [@global_tags_formatted, to_tags_list(message_tags)] |
There was a problem hiding this comment.
I was surprised to find out that Array#join flattens the provided array:
[1,[2,3]].join
# => "123"This actually came in handy, as it has proven faster than adding @global_tags_formatted to the array returned by to_tags_list(message_tags) or concatenating the result of to_tags_list(message_tags) to an existing array.
There was a problem hiding this comment.
That is interesting; it's like a flatten then regular join operation.
| @@ -44,11 +44,11 @@ | |||
|
|
|||
There was a problem hiding this comment.
Git diff doesn't interpret this diff very well: I've just reduced all values by one.
| @global_tags_formatted = @global_tags.join(',') if @global_tags.any? | ||
| end | ||
|
|
||
| def format(message_tags) |
There was a problem hiding this comment.
I remember doing something in this file ages ago, I think because Statsd was returning some format that we couldn't use downstream in the tracing library. I think we wanted to merge tags at the tracer library level into the global stats tags?
I would double check the history on this, and make sure it doesn't break tags downstream.
There was a problem hiding this comment.
I searched ddtraces history, and we don't access these global tags.
Regarding this in this repo, there are no modification attempts to global tags.
One thing that is technically possible today is to get a reference to it and mutate it:
stats = Datadog::Statsd.new
stats.tags << 'new:tag' # stats.tags is effectively +TagSerializer#global_tags+.Instead of using the public interface:
stats = Datadog::Statsd.new(tags: ['new:tag'])I'm not too convinced this should be supported behaviour by this library.
My suggestion is to freeze the internally stored TagSerializer#global_tags, thus having TagSerializer#global_tags guaranteed to return a read-only object.
There was a problem hiding this comment.
I think I agree with the sentiment of making global_tags immutable, from an interface perspective it feels intuitive to me.
| @global_tags_formatted = @global_tags.join(',') if @global_tags.any? | ||
| end | ||
|
|
||
| def format(message_tags) |
There was a problem hiding this comment.
I think I agree with the sentiment of making global_tags immutable, from an interface perspective it feels intuitive to me.
4 participants
While profiling the
ddtracegem, we noticed that enabling runtime metrics, which usesdogstatsd-ruby, caused some performance degradation.Running a small Rails application under ruby-prof showed us that the
TagSerializerandStatSerializerwere responsible for most of the time spend ondogstatsd-rubycalls.This PR improves the wall time and memory usage of the
TagSerializer.The largest gains came from processing the provided
message_tagsin a single pass and by caching a serialized version of the global tags.I've added a benchmark test to
tag_serializer_spec.rbthat allows one to replicate my results. This required the addition of benchmark-related gems to the:developmentgroup.Benchmark results
A detailed explanation of all benchmarked categories:
Before this change
After this change
Results