[Propagators] Improve JaegerPropagator performance#7171
[Propagators] Improve JaegerPropagator performance#7171martincostello merged 3 commits intoopen-telemetry:mainfrom
Conversation
Improve the performance of `JaegerPropagator` for .NET 8+ by using `SearchValues<T>`.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7171 +/- ##
==========================================
- Coverage 89.10% 89.08% -0.03%
==========================================
Files 271 271
Lines 13042 13063 +21
==========================================
+ Hits 11621 11637 +16
- Misses 1421 1426 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Pull request overview
This PR aims to improve JaegerPropagator extraction performance on modern .NET by optimizing how the propagator splits the uber-trace-id header into components.
Changes:
- Adds a .NET-specific fast-path for parsing header components using
SearchValues<char>+IndexOfAny. - Introduces a cached delimiter hint set (
:and%) for faster delimiter discovery. - Keeps the existing parsing implementation for non-.NET TFMs via conditional compilation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Do not silently scan past `%`.
|
Given the component is to be deprecated, should we do perf improvements for it? |
|
This was "free" as part of some performance investigation work I asked Copilot to do. I figured they were worth keeping rather than throwing away as we still ship this anyway. |
I don't think this is "free". While copilot did a lot of heavy lifting, the review cycle isn't free for us — approver bandwidth is one of our biggest bottlenecks across OTel repos. I'd rather we save that energy for higher-impact work, like accelerating the deprecation/removal of this component. Let's not make it harder for users to say goodbye to JaegerPropagator by making it faster! 🤣 |
|
If it had been a large change, I would have likely agreed, but for ~50 lines I considered it worth the improvement considering we still ship this package and it's mostly just adopting some newer APIs from .NET itself. |
rajkumar-rangaraj
left a comment
There was a problem hiding this comment.
I want to acknowledge @cijothomas's point. Approver bandwidth across the OTel repos is tight right now, and I agree we should be prioritizing the higher-impact items.
|
Next time I get Copilot to do some speculative search for improvements, I'll make sure I tell it to exclude any areas marked as deprecated. |
Changes
Improve the performance of
JaegerPropagatorfor .NET 8+ by usingSearchValues<T>.Benchmark Results
Copilot Summary
JaegerPropagator-perfimproves all commonExtractbenchmarks, whileInjectis mixed. Allocations are unchanged across every shared suite.mainJaegerPropagator-perfSampled=False, UseEncodedDelimiter=FalseSampled=False, UseEncodedDelimiter=FalseSampled=False, UseEncodedDelimiter=TrueSampled=False, UseEncodedDelimiter=TrueSampled=True, UseEncodedDelimiter=FalseSampled=True, UseEncodedDelimiter=FalseSampled=True, UseEncodedDelimiter=TrueSampled=True, UseEncodedDelimiter=TrueOverall
JaegerPropagator-perf, with the biggest wins in non-sampled cases (~54% lower duration).Detailed Results
Expand to view
main
This PR
Merge requirement checklist
Unit tests added/updatedAppropriateCHANGELOG.mdfiles updated for non-trivial changesChanges in public API reviewed (if applicable)