Skip to content
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

[HttpClient & ASP.NET Core] Document request durations #5079

Merged

Conversation

vishweshbankwar
Copy link
Member

@vishweshbankwar vishweshbankwar commented Nov 22, 2023

Fixes #3277
Design discussion issue #

Changes

Borrowing the definitions from https://learn.microsoft.com/en-us/dotnet/core/diagnostics/built-in-metrics-system-net#instrument-httpclientrequestduration and https://learn.microsoft.com/en-us/dotnet/core/diagnostics/built-in-metrics-aspnetcore#instrument-httpserverrequestduration as the implementations record the times around the same time when DiagnosticSource events are written.

For ASP.NET Core: Validated the metrics are recorded same time as DiagnosticSource callbacks are fired.
https://github.com/dotnet/aspnetcore/blob/5b1ad1ee4934be7d587e50670524876763cd45eb/src/Hosting/Hosting/src/Internal/HostingApplicationDiagnostics.cs#L156-L211

For HttpClient: Checked the handler configuration https://github.com/dotnet/runtime/blob/49d7bc83d986c32e8678001c841b1b836bc3e428/src/libraries/System.Net.Http/src/System/Net/Http/HttpClientHandler.cs#L40-L45 and also confirmed with @antonfirsov

Merge requirement checklist

  • CONTRIBUTING guidelines followed (nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@vishweshbankwar vishweshbankwar marked this pull request as ready for review November 28, 2023 22:09
@vishweshbankwar vishweshbankwar requested a review from a team as a code owner November 28, 2023 22:09
Co-authored-by: Anton Firszov <antonfir@gmail.com>
Copy link

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Client part looks good.

@TimothyMothra
Copy link
Contributor

Would it be useful to include the links to the learn.microsoft doc that defies the metric?
(ie: for more information see (link)).

@vishweshbankwar
Copy link
Member Author

Would it be useful to include the links to the learn.microsoft doc that defies the metric? (ie: for more information see (link)).

The link to metrics are already included in Readme. https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.Instrumentation.AspNetCore#list-of-metrics-produced

@utpilla utpilla merged commit 9211cee into open-telemetry:main Dec 1, 2023
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document the exact definition of durations
5 participants