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

Add histogram vectors to model #417

Merged
merged 13 commits into from
Jan 10, 2023
Merged

Conversation

zenador
Copy link
Contributor

@zenador zenador commented Dec 8, 2022

Addresses prometheus/client_golang#1179

Tested the JSON decoding successfully by scraping a simple server running native histograms with Prometheus, then using the HTTP API in client_golang (which does not need to be updated, only this library needs updating) to query it.

@zenador zenador force-pushed the sparsehistograms branch 3 times, most recently from 2c58700 to 63beb45 Compare December 8, 2022 13:00
@zenador zenador marked this pull request as ready for review December 8, 2022 14:59
@beorn7
Copy link
Member

beorn7 commented Dec 11, 2022

Cool. I hope I can help with a review soon…

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.

Thanks for doing this. Looks good overall.

I have identified a few issues, I believe. See comments. Let me know what you think.

(I'll do a detailed review once we have clarified those issues.)

model/value.go Outdated Show resolved Hide resolved
model/value_histogram.go Outdated Show resolved Hide resolved
model/value_histogram.go Outdated Show resolved Hide resolved
model/value_histogram.go Outdated Show resolved Hide resolved
model/value_histogram.go Outdated Show resolved Hide resolved
model/value.go Outdated Show resolved Hide resolved
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.

Looking good overall. I just have a lot of nits and agree with Beorn's comment about not requiring the Type field.

PS: this review does not cover the unit tests.

model/value.go Outdated Show resolved Hide resolved
model/value.go Outdated Show resolved Hide resolved
model/value.go Outdated Show resolved Hide resolved
model/value.go Outdated Show resolved Hide resolved
model/value.go Outdated Show resolved Hide resolved
model/value_histogram.go Outdated Show resolved Hide resolved
model/value_histogram.go Outdated Show resolved Hide resolved
model/value_histogram.go Show resolved Hide resolved
model/value_histogram.go Outdated Show resolved Hide resolved
model/value_histogram.go Show resolved Hide resolved
@zenador zenador force-pushed the sparsehistograms branch 4 times, most recently from 14ed1f8 to 57e36a0 Compare December 23, 2022 12:00
@beorn7
Copy link
Member

beorn7 commented Jan 3, 2023

Quick note: Bumping the minimum required Go version is a bit of an issue as prometheus/client_golang has to be conservative in this regard (and it uses prometheus/common). Currently, client_golang requires go1.17. Generally, they try to support the last three Go minor releases at the very least.

In short, I think, it would be better at this time to avoid generics and only require go1.17.

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.

Nice work! I just have a bunch more nits with some questions for Björn.

model/value.go Show resolved Hide resolved
model/value.go Show resolved Hide resolved
model/value.go Show resolved Hide resolved
model/value.go Outdated Show resolved Hide resolved
model/value_float.go Show resolved Hide resolved
model/value_histogram.go Show resolved Hide resolved
model/value_histogram.go Show resolved Hide resolved
model/value_histogram_test.go Show resolved Hide resolved
model/value_histogram_test.go Outdated Show resolved Hide resolved
model/value_histogram_test.go Outdated Show resolved Hide resolved
…g go version - experimental)"

This reverts commit 84e64de.

Signed-off-by: Jeanette Tan <[email protected]>
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.

LGTM. I will let @beorn7 take a final look.

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.

Great. Thank you. Just two commenting nits.

model/value.go Outdated Show resolved Hide resolved
model/value.go Show resolved Hide resolved
@beorn7
Copy link
Member

beorn7 commented Jan 10, 2023

I assume if @roidelapluie had issues with this, he would have chimed in by now.

So, merging now… Thank you very much, @zenador .

@beorn7 beorn7 merged commit 846591a into prometheus:main Jan 10, 2023
@zenador
Copy link
Contributor Author

zenador commented Jan 11, 2023

Thank you very much for the reviews, @beorn7 and @codesome !

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.

3 participants