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

Histogram buckets do not adjust for unit #5891

Open
ajatprabha opened this issue Oct 17, 2024 · 3 comments
Open

Histogram buckets do not adjust for unit #5891

ajatprabha opened this issue Oct 17, 2024 · 3 comments
Labels
blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made bug Something isn't working invalid This doesn't seem right

Comments

@ajatprabha
Copy link

ajatprabha commented Oct 17, 2024

Description

When I create a histogram without metric.WithUnit, it creates a prometheus histogram with buckets []float64{0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000} from metric.DefaultAggregationSelector I believe.

But when I use metric.WithUnit("s"), the buckets do not re-adjust to account for this unit change. This should either be documented or handled by the SDK. For example: SDK handles naming in such case, i.e. _milliseconds or _seconds.

Environment

  • OS: MacOS
  • Architecture: arm64
  • Go Version: 1.22.8
  • opentelemetry-go version: v1.30.0

Expected behavior

When I use metric.WithUnit("s") which overrides the default Unit ms, it should adjust the buckets to []float64{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}, if not set explicitly.

@ajatprabha ajatprabha added the bug Something isn't working label Oct 17, 2024
@dmathieu
Copy link
Member

Histogram buckets are defined by the specification.
https://github.com/open-telemetry/opentelemetry-specification/blob/0a78571045ca1dca48621c9648ec3c832c3c541c/specification/metrics/sdk.md#explicit-bucket-histogram-aggregation

Changing this behavior would require a specification change. Do you want to bring your use case there?

Note: I'm not sure this conversion would be the proper behavior. The default buckets come from defaults that make sense with ms. But if you change the unit, you likely want different values, not just a conversion by division.

@pellared pellared added invalid This doesn't seem right blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made labels Oct 17, 2024
@ajatprabha
Copy link
Author

ajatprabha commented Oct 18, 2024

In my experience, when different values are needed, one updates the default buckets anyway.

Whether changing Unit should result in scaling of buckets or not, that's a good point of discussion but using the same bucket bounds meant for milliseconds also doesn't make sense I think.
I mean 5s,10s,25s,50s... are quite big buckets.

At the very least, it is better to document this in godoc comments. WDYT?

@dmathieu
Copy link
Member

Yes, documenting this in godoc makes a lot of sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked:specification Waiting on clarification of the OpenTelemetry specification before progress can be made bug Something isn't working invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

3 participants