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: Identify native histograms even without observations #12584

Merged
merged 1 commit into from Jul 26, 2023

Conversation

beorn7
Copy link
Member

@beorn7 beorn7 commented Jul 20, 2023

Native histograms without observations and with a zero threshold of zero look the same as classic histograms in the protobuf exposition format. According to
prometheus/client_golang#1127 , the idea is to add a no-op span to those histograms to mark them as native histograms. This commit enables Prometheus to detect that no-op span and adds a doc comment to the proto spec describing the behavior.

@beorn7
Copy link
Member Author

beorn7 commented Jul 20, 2023

Companion PR for prometheus/client_golang#1314

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Just one typo.

@@ -1688,6 +1720,25 @@ func TestProtobufParse(t *testing.T) {
"__name__", "without_quantiles_sum",
),
},
{ // 78
m: "empty_histogram",
help: "A histogram without observations and with a zero threshold of zero but with a no-op spam to identify it as a native histogram.",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
help: "A histogram without observations and with a zero threshold of zero but with a no-op spam to identify it as a native histogram.",
help: "A histogram without observations and with a zero threshold of zero but with a no-op span to identify it as a native histogram.",

@@ -965,6 +978,25 @@ func TestProtobufParse(t *testing.T) {
"__name__", "without_quantiles_sum",
),
},
{
m: "empty_histogram",
help: "A histogram without observations and with a zero threshold of zero but with a no-op spam to identify it as a native histogram.",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
help: "A histogram without observations and with a zero threshold of zero but with a no-op spam to identify it as a native histogram.",
help: "A histogram without observations and with a zero threshold of zero but with a no-op span to identify it as a native histogram.",

@@ -517,6 +517,19 @@ metric: <
sample_sum: 1.234
>
>
`,
`name: "empty_histogram"
help: "A histogram without observations and with a zero threshold of zero but with a no-op spam to identify it as a native histogram."
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Suggested change
help: "A histogram without observations and with a zero threshold of zero but with a no-op spam to identify it as a native histogram."
help: "A histogram without observations and with a zero threshold of zero but with a no-op span to identify it as a native histogram."

@beorn7
Copy link
Member Author

beorn7 commented Jul 26, 2023

No-op spam would be nice indeed… :o) Will fix and then merge.

Native histograms without observations and with a zero threshold of
zero look the same as classic histograms in the protobuf exposition
format. According to
prometheus/client_golang#1127 , the idea is
to add a no-op span to those histograms to mark them as native
histograms. This commit enables Prometheus to detect that no-op span
and adds a doc comment to the proto spec describing the behavior.

Signed-off-by: beorn7 <beorn@grafana.com>
@beorn7 beorn7 merged commit b1a6d00 into main Jul 26, 2023
40 checks passed
@beorn7 beorn7 deleted the beorn7/histogram branch July 26, 2023 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants