Optimize overrideTags for objects #5
Conversation
wesmoncrief
changed the title
lib/statsd.js
Outdated
| else if (Object.keys(tags).length === 0) { | ||
| return globalTagsMemo; | ||
| } | ||
| if (isTagsArray || Array.isArray(globalTags)) { |
There was a problem hiding this comment.
the lack of typing here (not really your fault) makes it really hard to follow what the assumptions here are... to is it possible for tags to be an array but for globalTags to be an object? or the other way around?
does overrideTagsToString work if you pass in objects for globalTags or tags?
There was a problem hiding this comment.
tags could be array or object. globalTags could be array or object. Welcome to the wild west.
overrideTagsToString does work for any combination of objects/arrays.
overrideTags2 only works if globalTags is an object, and also tags is an object
There was a problem hiding this comment.
btw, the unit tests seem pretty good, but i was thinking i'd also test this by testing on a manual image deployment of xover in a small staging region
lib/helpers.js
Outdated
| for (const g of Object.keys(globalTags)) { | ||
| if (!usedGlobalKeys.has(g)) { | ||
| const serializedTagWithValue = `${g}:${sanitizeTags(globalTags[g], telegraf)}`; | ||
| result = serializedTagWithValue + separator + result; |
There was a problem hiding this comment.
should we use addToResult(serializedTagWithValue, true) here?
A couple of optimizations here. These work for when you have tags as objects, not arrays.
overrideTagsUnoptimizedis slow because it does a lot of string manipulation - it serializes then unserializes tag/value pairs.overrideTags2can avoid this by only taking in objects. So this optimization is only able to be used when the caller passes in objects (instead of arrays).benchmark results: