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

support native histograms #169

Merged
merged 1 commit into from
Jun 26, 2024
Merged

Conversation

jan--f
Copy link
Contributor

@jan--f jan--f commented May 31, 2024

Fixes: #125

@jan--f jan--f requested a review from beorn7 May 31, 2024 08:32
@jan--f
Copy link
Contributor Author

jan--f commented May 31, 2024

Not sure if pulling in the pushgateway dependency is acceptable. should be easy enough the copy the code if that's preferable.

Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you very much.

Just the dependency on the pushgateway seems really out of place.

Thinking about it, we cannot share code from prometheus/prometheus, because the latter is using gogo-protobuf, while we are using the official Go protobuf library via prometheus/client_model. Furthermore, prometheus/prometheus is using jsoniter for performance reason, while we are using just vanilla json marshaling here.

In the not too far future, we'll migrate prometheus/prometheus to a still maintained high-performance protobuf library. Maybe that would be a good time to also move all our other protobuf usage to that same, yet to be chosen library… From that perspective, the protobuf related code here is temporary, and it would maybe OK to just copy it for the time being.

Alternatively, maybe a histogram subdirectory in prometheus/common/model might be a good place to share the code.

My gut feeling is, let's copy for now.


require (
github.com/davecgh/go-spew v1.1.1
github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc
Copy link
Member

Choose a reason for hiding this comment

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

Is this update (to a specific hash) required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't specifically update this. I'll copy the pushgateway for now, that should fix this dependency update too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comes via the prometheus/prometheus dependency.

Copy link
Member

Choose a reason for hiding this comment

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

OK, so not our fault… 🤷

@beorn7
Copy link
Member

beorn7 commented Jun 4, 2024

Another thing: Could you also update https://github.com/prometheus/prom2json?tab=readme-ov-file#json-format with a section that contains a native histogram?

@jan--f
Copy link
Contributor Author

jan--f commented Jun 21, 2024

Thinking about it, we cannot share code from prometheus/prometheus, because the latter is using gogo-protobuf, while we are using the official Go protobuf library via prometheus/client_model. Furthermore, prometheus/prometheus is using jsoniter for performance reason, while we are using just vanilla json marshaling here.

Hmm not sure I understand how protobuf factors into this. The histogram_model code in pushgateway imports both github.com/prometheus/client_model/go and github.com/prometheus/prometheus/model/histogram specifically to go from the client model type to the prometheus model type. I wanted to avoid duplicating the Histogram type as defined in github.com/prometheus/prometheus/model/
Is this code different or should I not have done this in pushgatway either?

@jan--f jan--f force-pushed the native-histograms branch 2 times, most recently from f7dc838 to adb4a9d Compare June 21, 2024 08:59
Signed-off-by: Jan Fajerski <jfajersk@redhat.com>
@beorn7
Copy link
Member

beorn7 commented Jun 26, 2024

You are doing the right thing WRT protobuf.

I was just hoping we could re-use even more code, but prometheus/prometheus is using https://github.com/prometheus/prometheus/blob/main/prompb/io/prometheus/client/ rather than github.com/prometheus/client_model/go when it uses the Go types autogenerated frem the proto spec.

Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

Thank you very much.

@beorn7 beorn7 merged commit dad8bfb into prometheus:master Jun 26, 2024
4 checks passed
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.

Add support for native histograms
2 participants