- 
                Notifications
    You must be signed in to change notification settings 
- Fork 5.2k
HttpClientHandler request metrics #87319
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
          
     Merged
      
      
    
  
     Merged
                    Changes from all 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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?
Uh oh!
There was an error while loading. Please reload this page.
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
Meterwhich could be implemented in many ways inSystem.Diagnostics.Metrics.Can't those also be implemented by extending types in
System.Diagnostics.Metricsin a backwards-compatible manner? (eg. newprotected virtualmethods)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.