Memoize environment tags to improve performance #1762
Conversation
fermayo
added
bug
| @@ -38,7 +38,10 @@ def self.set_tags!(span, tags = {}) | |||
| span.context.origin = Ext::Test::CONTEXT_ORIGIN if span.context | |||
There was a problem hiding this comment.
⬆️ GitHub doesn't allow me to comment on line 34, but is it just me or is it kinda weird that this method can receive an arbitrary tags hash, but does not actually set all of the tags in it, instead just cherry picking a few?
There was a problem hiding this comment.
AFAIK it uses all tags passed in, but then adds/overwrites them with metadata. This should be an internal API anyway used only by the CI auto-instrumentation.
|
@ivoanjo we have internal end-to-end performance tests that do this already. What would you suggest doing? A benchmark? |
There was a problem hiding this comment.
@ivoanjo we have internal end-to-end performance tests that do this already. What would you suggest doing? A benchmark?
I was thinking more of a test that checks that Ext::Environment.tags(ENV) only ever gets called only once.
But if the internal end-to-end tests already cover this comfortably, then I'm very OK with relying on that -- up to you :)
|
@ivoanjo oh that makes sense! I'm not familiar with the testing of this code, but we will definitely follow up with a test to avoid regressions. |
|
🎉 |
Fixes #1748
Environment tags do not change during the life of the test process, so we should calculate them only once.