-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Remove HttpMetricsEnrichmentContext caching #110580
Conversation
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.
Awesome, thanks!
/backport to release/9.0-staging |
Started backporting to release/9.0-staging: https://github.com/dotnet/runtime/actions/runs/12284286138 |
/backport to release/8.0-staging |
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/12285016129 |
Fixes #108284
With this change the
HttpMetricsEnrichmentContext
is only used while we're running the callbacks, we're no longer storing it on the request properties.As it's now much more short lived, and doesn't carry as much state (no
List
s), there's no point in pooling it.The change does introduce a bit more allocations as we're now allocating a
List<Callback>
per request.If we care, we can add the extra logic to avoid the list allocation when there's just 1 callback.
I'm also using the
TagList
directly since we're not pooling theList
anymore. I think this should still be fine since its default capacity is 8 before allocating more, and we're only adding ~5 tags, so there's room for user-provided ones.