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 aggregation should support Min Max #2560

Closed
cijothomas opened this issue Nov 3, 2021 · 10 comments · Fixed by #2735
Closed

Histogram aggregation should support Min Max #2560

cijothomas opened this issue Nov 3, 2021 · 10 comments · Fixed by #2735
Labels
enhancement New feature or request metrics Metrics signal related
Milestone

Comments

@cijothomas
Copy link
Member

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#explicit-bucket-histogram-aggregation
It should be collected by default, with the ability to turn off.

@cijothomas cijothomas added enhancement New feature or request metrics Metrics signal related labels Nov 3, 2021
@mic-max
Copy link
Contributor

mic-max commented Nov 4, 2021

I'm interested in helping with this item with some guidance 🙂

@cijothomas
Copy link
Member Author

Sure. I wanted to wait till open-telemetry/opentelemetry-proto#279 is merged. But we don't have to wait really for that.

@cijothomas cijothomas added this to the 1.3.0 milestone Nov 18, 2021
@cijothomas cijothomas modified the milestones: 1.3.0, 1.4.0 May 27, 2022
@cijothomas
Copy link
Member Author

Removing this from 1.3.0 milestone and moving to 1.4.0.
1.3.0 is releasing shortly, and want to allow bake time for the new feature.

@WenheLI
Copy link

WenheLI commented Jun 9, 2022

Hi, the previous PR seems to be closed due to inactivity.
I am just wondering if I could take that over?

@cijothomas
Copy link
Member Author

Hi, the previous PR seems to be closed due to inactivity. I am just wondering if I could take that over?

If @mic-max is not planning to work on it, then yes. @mic-max could you respond here?

@mic-max
Copy link
Contributor

mic-max commented Jun 29, 2022

Hi, the previous PR seems to be closed due to inactivity. I am just wondering if I could take that over?

If @mic-max is not planning to work on it, then yes. @mic-max could you respond here?

Yes, I plan to work on it this week, sorry I missed this notification. It should be a fairly simple fingers crossed rebase and touch up to the existing PR :)

@YarekTyshchenko
Copy link

Are there any docs on how to make this work with the PrometheusExporter? I can't figure out if there is anything special to do here or the exporter. The Metric.CreateHistogram<T> has no settings for buckets or inclusion of min/max.

@cijothomas
Copy link
Member Author

Are there any docs on how to make this work with the PrometheusExporter? I can't figure out if there is anything special to do here or the exporter. The Metric.CreateHistogram<T> has no settings for buckets or inclusion of min/max.

MinMax is not supported for PrometheusExporter. The spec does not mention how to convert Min/Max for Prometheus.
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/compatibility/prometheus_and_openmetrics.md#histograms-1

@YarekTyshchenko
Copy link

YarekTyshchenko commented Apr 21, 2023

Thanks for clarification @cijothomas. Do you have any advice on how to proceed? I'm used to using Histogram data type for gauge type metrics like "latency" or "delay" where max data is important. The data itself is a gauge, but missing all the data in between the sampling points is undesirable.

It looks like with OpenTelemetry there's no datatype that would give me this data

Edit: To reference a statsd analog that I've used before: https://github.com/statsd/statsd/blob/master/docs/metric_types.md#timing This is perhaps not well named, as this type is useful for any "gauge-like" metric, where you are interested in all data, and not just the last at time of sampling.

@cijothomas
Copy link
Member Author

I don't fully understand the issue you are facing. If the issue is Prometheus Exporter missing min/max, then this is just a matter of OTel spec clarifying how to map Min/Max to Prometheus model (likely a Prometheus Gauge). Please open an issue in the spec repo for this.

If it is something else, could you elaborate/clarify the exact ask?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request metrics Metrics signal related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants