-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
HttpClientHandler request metrics #87319
Merged
Merged
Changes from 19 commits
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
ec16299
initial implementation of request metrics
antonfirsov 2bacea0
simplify error handling code and add comments
antonfirsov 74597a4
typo
antonfirsov 12af084
fix Http.Unit.Tests build failure
antonfirsov 528f3b1
wip
antonfirsov 0d97a7d
Merge branch 'main' into MetricsHandler-02
antonfirsov 4fcfee6
callback-based enrichment API with reftype context
antonfirsov bd461c8
Protect against re-reentrancy
antonfirsov 3c80a0b
use shared counters with the shared meter
antonfirsov 7406c46
do not cache instruments already cached by Meter
antonfirsov 9da9485
use [ThreadStatic] instead of ThreadLocal
antonfirsov 9251264
Merge branch 'main' into MetricsHandler-05
antonfirsov 2a90c07
sync with main
antonfirsov 869bd34
review feedback
antonfirsov 1d43c3e
use FrozenDictionary for caching status codes
antonfirsov d4eadb1
Merge branch 'main' into MetricsHandler-02
antonfirsov 493e7d8
implement final design
antonfirsov d049d4f
fixup merge
antonfirsov 6d4cd18
rename "contextCache" to "pool"
antonfirsov 9f4dd8e
comments
antonfirsov df6189e
Simplify status code cache
antonfirsov bbc38af
Merge branch 'MetricsHandler-02' of https://github.com/antonfirsov/ru…
antonfirsov cd5e4a4
review feedback
antonfirsov 62f87b2
move instrument names to constants in tests
antonfirsov 6283c24
harden HttpMetricsTest_DefaultMeter
antonfirsov 6b77359
remove unnecessary reference to System.Collections.Immutable
antonfirsov f61c85b
lazy-init s_boxedStatusCodes
antonfirsov 9eb314e
oops
antonfirsov 8e35e19
simplify HttpMetricsTest_DefaultMeter.RequestDuration_Success_Recorde…
antonfirsov 1e7453f
move the default meter tests out of process
antonfirsov e686bee
oops
antonfirsov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -289,7 +289,6 @@ private static HttpResponseMessage ConvertResponse(HttpRequestMessage request, W | |
|
||
protected internal override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) | ||
{ | ||
ArgumentNullException.ThrowIfNull(request); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is addressing concerns from #87319 (comment) by consolidating all null-checks to |
||
bool? allowAutoRedirect = _isAllowAutoRedirectTouched ? AllowAutoRedirect : null; | ||
#if FEATURE_WASM_THREADS | ||
return JSHost.CurrentOrMainJSSynchronizationContext.Send(() => | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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'm not proposing any change to the plan around this new API, but I did want to share info that came up in some very recent discussions so that everyone is on the same page. Maybe someone will feel differently than I do.
When we added this @stephentoub and @bartonjs were sensitive whether the Meter API is here to stay and my opinion hasn't changed on that. However in some discussions with partners around metrics naming its possible we'll get a future request to do for metrics what @CarnaViire is doing now for HttpClient logging. In the same way that @CarnaViire's feature doesn't mean .NET lessened our support for the ILogger interface, a configurable metrics feature would also still be reliant on Meter. What it might mean however is that rather than passing a MeterFactory to some hard-coded logic inside the HttpClientHandler, now some scenarios would be passing a delegate or an interface that could run some custom code. They might use that delegate to capture different metrics or name the metrics differently. A Meter would still be getting used to produce the metrics, but it might become an implementation detail as a field in the custom delegate closure. I'd guess its 50/50 that we need something like that in the next 5 years. If that did happen folks might ask in hindsight did we need this MeterFactory property or could we have just started with the custom delegate/interface to begin with. However I think the challenge of attempting a broader abstraction is I don't have clarity on exactly what requirements are going to emerge. I could guess but we both might over-engineer for requirements that never come about or under-engineer and we still need to come back to add a different interface later.
I'm happy with how this API looks right now even if we do wind up needing to add more configurability in a separate future feature. Do others feel the same way?
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.
My personal hot-take on this is that it would be reasonable put a big EXPERIMENTAL label on all the metrics features and API-s we are introducing in 8.0 because of all the uncertainties and the fact that OTel semantic conventions standard we are trying to conform to is also not stable yet.
I really wish we can avoid that. It looks to me that all that the most important thing we may need in the future is a name-mapping mechanism in
Meter
which could be implemented in many ways inSystem.Diagnostics.Metrics
.Can't those also be implemented by extending types in
System.Diagnostics.Metrics
in a backwards-compatible manner? (eg. newprotected virtual
methods)Or is there something I'm missing?
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 am just anticipating that we may be asked for customizations that include new metrics, or things that are differently named, or the same names but differently measured. If the changes were non-breaking and apply to everyone then yes we could implement directly in HttpClient. But if it is people wanting customizations not suitable for everyone then we'd probably start looking at extensibility. I'm not advocating we add these things nor am I at all confident these requests will ever come, but I did want to put cards on the table that its at least a possibility.
Based on recent discussions with Reiley they are stable (enough) and I am planning to put together a PR shortly that updates our names to match.