Skip to content

Histogram Record operation should be explicit about not recording negative values #2757

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

Closed
rapphil opened this issue Aug 29, 2022 · 5 comments
Assignees
Labels
area:api Cross language API specification issue spec:metrics Related to the specification/metrics directory triage:rejected:needs-info

Comments

@rapphil
Copy link

rapphil commented Aug 29, 2022

What are you trying to achieve?

I'm trying to make sure that all the languages are consistent with regards to the implementation of the explicit bucket histogram.

The specification should be clear about the expected behavior of implementations when a negative number is passed to the Record operation of a Histogram.

Right now it says:

The amount of the Measurement, which MUST be a non-negative numeric value.

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/api.md#record

What did you expect to see?

I expect the text to be clear.

Suggestion:

The amount of the Measurement, which MUST be a non-negative numeric value. Negative numbers MUST be ignored and an optional error message might be reported.

Additional context.

As @reyang pointed out, this might be a bigger issue since Exponential histograms do support negative numbers. Maybe a solution is to move the requirements with regards to the parameter of the Record operation of Histograms to the SDK metrics specification, so that we can have a definition for Explicit Bucket histograms and another for Exponential Histograms?

@rapphil rapphil added the spec:metrics Related to the specification/metrics directory label Aug 29, 2022
@arminru arminru added the area:api Cross language API specification issue label Aug 30, 2022
@rbailey7210 rbailey7210 added the [label deprecated] triaged-accepted [label deprecated] Issue triaged and accepted by OTel community, can proceed with creating a PR label Sep 2, 2022
@jmacd
Copy link
Contributor

jmacd commented Sep 2, 2022

The suggested clarification is welcome. As for the additional context, I feel that support for negative observations is a post-1.0 issue to be addressed.

@jsuereth
Copy link
Contributor

I think for 1.0, Histogram instruments should explicitly disallow negative measurements, as @jmacd suggests. Please submit a PR if you have time!

@reyang
Copy link
Member

reyang commented Sep 12, 2022

Related to #2770 (we might want to remove 0 from the default bounds).

@breedx-splk
Copy link
Contributor

@rapphil I agree with @jmacd and think that your suggestion is a good one. Would you be able to submit a PR with that change?

@danielgblanco danielgblanco added triage:rejected:needs-info and removed [label deprecated] triaged-accepted [label deprecated] Issue triaged and accepted by OTel community, can proceed with creating a PR labels Jun 3, 2024
@danielgblanco
Copy link
Contributor

Hi @rapphil we see the language was changed in the spec related to histogram measurements. We have seen no feedback on the issue and we think it'd require more info to get it reconsidered. I'm going to close this issue, but if you think this is still an issue, please feel free to leave a comment and we'll re-open it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:api Cross language API specification issue spec:metrics Related to the specification/metrics directory triage:rejected:needs-info
Projects
None yet
Development

No branches or pull requests

8 participants