tracer: WithHeaderTags performance improvements #2117
Conversation
BenchmarksBenchmark execution time: 2023-07-18 17:04:41 Comparing candidate commit aef95ea in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 27 metrics, 0 unstable metrics. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Refactor to use generics and add a type alias for clarity
ajgajg1134
force-pushed
the
andrew.glaude/WithHeaderTagsPerf
branch
from
2085373 to
33b7fee
Compare
|
Bench results of withoutFix vs WithFix shows a measurable ~4% improvement. For users with more http headers in their requests (likely most customers, but this is a guess), this improvement will be larger. |
ajgajg1134
changed the title
internal/utils.go
Outdated
| func (r *ReadOnlyLockMap) Clear() { | ||
| panic("Not safe to call Clear on a ReadOnlyLockMap") | ||
| } |
There was a problem hiding this comment.
🤔 I'm usually not a fan of types that pretend to implement an interface.
Do we need an interface at all? Is there harm in just having a single LockMap struct type that implements all of these? The performance hit from an uncontested rlock should be trivial.
Alternatively, if we think it's really important, I'd rather have a ROMap interface that only specifies Iter and Len. Then the contribs can use that type, and both the ReadOnlyLockMap with Clear() removed (and probably renamed to ReadOnlyMap) and the globalconfig.headerTagMap will satisfy that interface. globalconfig can just continue to use the headerTagMap and its Clear method directly.
|
With latest changes the bench is even better (nearly 6% vs 4% previously): |
ajgajg1134
force-pushed
the
andrew.glaude/WithHeaderTagsPerf
branch
from
aef95ea to
b84fd3b
Compare
|
|
||
| // LockMap uses an RWMutex to synchronize map access to allow for concurrent access. | ||
| // This should not be used for cases with heavy write load and performance concerns. | ||
| type LockMap struct { |
…cherrypick #2117) (#2125) Previously, we iterated over every http header and called strings.Lower on every single one, this incurred additional CPU cost, especially for services with many http headers in their traced requests, even for users who are not using the "WithHeaderTags" functionality. Instead of looping over every request header, loop over the configured header->tag map so that performance is not affected when this feature is not in use (and this should also be faster for users of this feature). This should be a refactor of customer facing functionality, this is thoroughly covered by the existing unit tests and there is a new benchmark to ensure we don't regress in the future (can be added to benchmarking platform after merging to main)
What does this PR do?
Instead of looping over every request header, loop over the configured header->tag map so that performance is not affected when this feature is not in use (and this should also be faster for users of this feature).
Motivation
Previously, we iterated over every http header and called strings.Lower on every single one, this incurred additional CPU cost, especially for services with many http headers in their traced requests, even for users who are not using the "WithHeaderTags" functionality.
Describe how to test/QA your changes
This should be a refactor of customer facing functionality, this is thoroughly covered by the existing unit tests and there is a new benchmark to ensure we don't regress in the future (can be added to benchmarking platform after merging to main)
Reviewer's Checklist