-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Add System.Diagnostics.TextMapPropagator support #33777
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This comment has been minimized.
This comment has been minimized.
Plaintext on
|
@Tratcher The only new change I've made since we last spoke is to resolve the Propagator from DI instead of using the global static. Can you review that? Also, the difference in the GC counters makes no sense to me 🤷. Can you help me make sense of that? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DI change seems fine
No idea on the GC data, but at least it's getting better and not worse 😁. |
There's no way LOH is 672 bytes. Kestrel's slabbed memory pool is allocated in the LOH. @sebastienros any ideas? Repeating the run, I got 131800 for Max LOH size for both before/after. As for reduction in Max Gen 1 and increase in Max Gen 2 size, I'm not sure what to make of it? Something is getting promoted sooner? I'll try collecting an allocation profile to make more sense of this |
d476798
to
abcc758
Compare
src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs
Outdated
Show resolved
Hide resolved
src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs
Outdated
Show resolved
Hide resolved
src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs
Outdated
Show resolved
Hide resolved
src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs
Outdated
Show resolved
Hide resolved
May be a clever idea in a future PR to add a test consuming other propagator. e.g., tests create a custom propagator and consume it. You don't have to block this PR on that. Refers to: src/Hosting/Hosting/test/HostingApplicationTests.cs:182 in abcc758. [](commit_id = abcc758, deletion_comment = False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some commented. LGTM otherwise.
// AddBaggage adds items at the beginning of the list, so we need to add them in reverse to keep the same order as the client | ||
// An order could be important if baggage has two items with the same key (that is allowed by the contract) | ||
for (var i = baggage.Length - 1; i >= 0; i--) | ||
// Order could be important if baggage has two items with the same key (that is allowed by the contract) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You used to add these in reverse, does ExtractBaggage handle the reversal? If so then include that in the comment. Otherwise this comment about order sensitivity doesn't make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ll send a follow-up PR to fix the comment. I want to merge this when the PR checks are green in case AzDo has a bad day again
I'm assuming there's no extra allocations here. |
No description provided.