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

prompb: Update exposition protobuf to include float and gauge histograms #10932

Merged
merged 1 commit into from Jun 30, 2022

Conversation

beorn7
Copy link
Member

@beorn7 beorn7 commented Jun 29, 2022

This is in preparation of enabling ingestion of float and gauge histograms. The actual changes have happened in prometheus/client_model#58 , but since we use proto3 and gogoprotobuf in Prometheus, the changes have to be pulled in manually here.

@marctc

@beorn7 beorn7 requested a review from codesome June 29, 2022 16:47
@beorn7 beorn7 changed the title prompb: Update expositios protobuf to include float and gauge histograms prompb: Update exposition protobuf to include float and gauge histograms Jun 29, 2022
Copy link
Member

@codesome codesome left a comment

Choose a reason for hiding this comment

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

If I read it correctly, you added the float counts at 4 places, some additional comments, and some changes in the proto formatting.

MetricType_HISTOGRAM MetricType = 4
// GAUGE_HISTOGRAM must use the Metric field "histogram".
Copy link
Member Author

Choose a reason for hiding this comment

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

No, that's the actual trick here: There is no field gauge_histogram. There is only one field histogram in Metric. Whether it's a counter histogram or a gauge histogram is solely determined by this enum.

@beorn7
Copy link
Member Author

beorn7 commented Jun 30, 2022

Yeah, the re-formatting creates a lot of diff noise.

This should create/decode the same proto messages on the wire as prometheus/client_model#58 .

@marctc I'll merge this now. You can start working on ingestion, as discussed. Just no-op storing the FloatHistogram for now (with a TODO) and add an enum to Histogram and FloatHistogram to store the "counter-reset"/"no-counter-reset"/"don't know"/"gauge histogram" information. Let's call it ResetHint (as in the remote-write protobuf). I think there should be no active counter reset detection during scraping. It will only be "don't know" or "gauge histogram". (During actually storing the histogram, a counter reset is detected for the don't know case, which is already implemented. Just needs change for the new "gauge histogram" case.)

@beorn7 beorn7 merged commit 1dc732c into sparsehistogram Jun 30, 2022
@beorn7 beorn7 deleted the beorn7/protobuf branch June 30, 2022 08:44
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

3 participants