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

correct metric type for http req/res sizes #5141

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

jamesmoessis
Copy link
Contributor

@jamesmoessis jamesmoessis commented Sep 3, 2024

It took me a while to find out these docs were wrong, reading the OTLP receiver code, then the obs reporter helper, then the opentelemetry-go-contrib which holds the instrumentation for the HTTP servers. Thought I would save someone else the time.

These are counters, not histogram. Here is the code that records this: link

Interestingly, the gRPC code uses a histogram, so I've left it: link

@jamesmoessis jamesmoessis requested a review from a team September 3, 2024 07:21
@opentelemetrybot opentelemetrybot requested review from a team and bogdandrutu and removed request for a team September 3, 2024 07:25
Copy link
Member

@theletterf theletterf left a comment

Choose a reason for hiding this comment

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

@open-telemetry/collector-approvers Please have a look.

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

LGTM. IIRC, there's an issue open with the underlying Go SDK to make this a histogram, so this might get outdated soon again (to the state before this PR), but this PR reflects the current situation of the Collector.

@theletterf theletterf added this pull request to the merge queue Sep 3, 2024
Merged via the queue into open-telemetry:main with commit e32c31e Sep 3, 2024
17 checks passed
@jamesmoessis jamesmoessis deleted the fix-metric-type branch September 4, 2024 03:14
@jamesmoessis
Copy link
Contributor Author

Thankyou @jpkrohling!

@jamesmoessis
Copy link
Contributor Author

For what it's worth, a counter here is more useful for me because I want to know total bytes rather than distribution over requests. I can see use cases for both. This is probably not the right place to discuss it, though.

michael2893 pushed a commit to michael2893/opentelemetry.io that referenced this pull request Sep 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants