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.

Sign up for GitHub

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

HTTP duration metrics should match HTTP span duration #69

Merged
merged 2 commits into from
Jun 2, 2023
Merged

HTTP duration metrics should match HTTP span duration #69

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2441206
HTTP duration metrics should match HTTP span duration
trask Jun 1, 2023
4f9eb02
Merge branch 'main' into http-metrics
trask Jun 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,3 +70,5 @@ release.
([#54](https://github.com/open-telemetry/semantic-conventions/pull/54))
- Add `gcp.gce.instance.name` and `gcp.gce.instance.hostname` resource
attributes for GCP Compute Engine VMs. ([#15](https://github.com/open-telemetry/semantic-conventions/pull/15))
- Add note that HTTP duration metrics should match HTTP span duration.
([#69](https://github.com/open-telemetry/semantic-conventions/pull/69))
4 changes: 4 additions & 0 deletions specification/metrics/semantic_conventions/http-metrics.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ operations. By adding HTTP attributes to metric events it allows for finely tune

This metric is required.

When this metric is reported alongside an HTTP server span, the metric value SHOULD be the same as the HTTP server span duration.
Copy link
Member

Choose a reason for hiding this comment

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

While I 100% agree with this statement, I think that the current shape of the API does not allow us to implement it. We can't retrieve start/end timestamps from a Span without converting it to SpanData first (because of the read/write API split), which is very wasteful. In the Java instrumentation we've decided to use current time at start/end of the operation (unless the start/end timestamps are explicitly provided by the instrumentation), and they are always a bit off compared to the span timestamps.

Also see open-telemetry/opentelemetry-java-instrumentation#5905 for a bit more context

Copy link
Member

Choose a reason for hiding this comment

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

While I 100% agree with this statement, I think that the current shape of the API does not allow us to implement it.

Correct, and that's exactly one of the reason why "SHOULD" is used rather than "MUST".

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. It does seem a bit weird that we're adding a recommendation that can't be fulfilled at the moment, but I suppose that leaves us an option to fix that in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

ya, good point, I guess it's more "aspirational" and hopefully will drive improvement in the SDK to make it a reality


This metric SHOULD be specified with
[`ExplicitBucketBoundaries`](https://github.com/open-telemetry/opentelemetry-specification/tree/v1.21.0/specification/metrics/api.md#instrument-advice)
of `[ 0, 0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10 ]`.
Expand Down Expand Up @@ -241,6 +243,8 @@ SHOULD NOT be set if only IP address is available and capturing name would requi

This metric is required.

When this metric is reported alongside an HTTP client span, the metric value SHOULD be the same as the HTTP client span duration.

This metric SHOULD be specified with
[`ExplicitBucketBoundaries`](https://github.com/open-telemetry/opentelemetry-specification/tree/v1.21.0/specification/metrics/api.md#instrument-advice)
of `[ 0, 0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10 ]`.
Expand Down