Make LegacyPropagator list sort linear - #132373
Conversation
Replace O(n^2) behaviour with O(n) to reverse the list of keys parsed from the baggage header by `LegacyPropagator`, matching the implementation in `W3CPropagator`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Improves LegacyPropagator baggage extraction performance by avoiding repeated List.Insert(0, …) (quadratic behavior) and instead appending entries then reversing once, aligning the ordering behavior with W3CPropagator’s extraction logic. Updates existing propagator tests so baggage ordering expectations are exercised with multiple entries.
Changes:
- Replace per-entry
Insert(0, …)withAdd(…)plus a singleReverse()at the end ofLegacyPropagator.TryExtractBaggage. - Expand legacy-propagator test inputs to include multiple baggage entries so reverse-order behavior is actually validated.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/LegacyPropagator.cs | Switches baggage list construction to Add + one-time Reverse() to avoid O(n²) behavior while preserving ordering semantics. |
| src/libraries/System.Diagnostics.DiagnosticSource/tests/PropagatorTests.cs | Adds additional baggage entries to legacy propagator test cases so ordering/reversal is meaningfully asserted. |
|
I don't think we need to change the legacy propagator and risk any app compatibility issues. We introduced the W3CPropagator to be more compliant with the specifications, and it is now the default. Additionally, making the legacy propagator behave like the W3CPropagator is not a goal. Otherwise, there would have been no reason to introduce a separate W3CPropagator in the first place. Please let's know why not using W3CPropagator is enough. CC @noahfalk |
|
The goal is purely to improve the performance for users who may still be using the legacy implementation for whatever reason, as noted in the description. There should be zero observable difference other than time. |
|
@martincostello, thanks. Did you encounter any complaints about the performance there, or are you just doing this proactively? My point is this is really legacy and not the default anymore. The benefit is not that worth to touch the code there. |
|
No, I had Copilot look at the o11y-related code to see if there was anything it could find that was a low-cost optimisation that could be made, which surfaced #132368 and this. The change seemed a no-brainer to me when I reviewed the findings considering that in terms of implementation, disregarding any propagation differences between W3C and the previous implementation of how baggage propagates, it's the same (insert in order, then reverse at the end). |
|
Okay, let's wait for the snap for RC1 to be done, and then I can review and merge it after that, including #132368. |
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Replace O(n^2) behaviour with O(n) to reverse the list of keys parsed from the baggage header by
LegacyPropagator, matching the implementation inW3CPropagator.Also adds a test to verify the ordering.
Impact in the default case is minimised by Kestrel and IIS' HTTP request header limits (8KB and 16KB) and the legacy propagator being opt-in in .NET 10+.