prometheusbp: add support for native histograms #709
Conversation
Xaelias
force-pushed
the
chore_-_adhoc_-_2026-01-12_-_add_support_for_native_histograms
branch
from
69e31fa to
b06a4cd
Compare
Xaelias
requested review from
konradreiche,
mathyourlife-reddit and
pacejackson
and removed request for
a team
|
|
||
| import "time" | ||
|
|
||
| const ( |
There was a problem hiding this comment.
Not a golang expert but I believe we want these to be const?
There was a problem hiding this comment.
If they can be declared as constants, it's definitely preferable.
There was a problem hiding this comment.
there is one service that is overriding the prometheusbp.DefaultLatencyBuckets. maybe remove the Default prefix if these are not intended to be overridden directly.
konradreiche
left a comment
konradreiche
left a comment
There was a problem hiding this comment.
Looks good to me. What would it look like to test those changes in an existing or new Go test?
|
|
||
| import "time" | ||
|
|
||
| const ( |
There was a problem hiding this comment.
If they can be declared as constants, it's definitely preferable.
|
|
||
| import "time" | ||
|
|
||
| const ( |
There was a problem hiding this comment.
there is one service that is overriding the prometheusbp.DefaultLatencyBuckets. maybe remove the Default prefix if these are not intended to be overridden directly.
Xaelias
force-pushed
the
chore_-_adhoc_-_2026-01-12_-_add_support_for_native_histograms
branch
3 times, most recently
from
b1d7c95 to
5bf74f1
Compare
Add support for native histograms using the following settings: - NativeHistogramBucketFactor: 1.1 - NativeHistogramMaxBucketNumber: 160 - NativeHistogramMinResetDuration: 1h
Xaelias
force-pushed
the
chore_-_adhoc_-_2026-01-12_-_add_support_for_native_histograms
branch
3 times, most recently
from
6c05835 to
86ba4be
Compare
Xaelias
force-pushed
the
chore_-_adhoc_-_2026-01-12_-_add_support_for_native_histograms
branch
from
86ba4be to
dd13d98
Compare
kylelemons
left a comment
kylelemons
left a comment
There was a problem hiding this comment.
Looks good to me, but def make sure to sync with the Baseplate team before merging
|
|
||
| // DefaultNativeHistogramMinResetDuration is the default minimum duration between | ||
| // resets for native histograms. | ||
| DefaultNativeHistogramMinResetDuration = time.Hour |
There was a problem hiding this comment.
🔕 (nit) I like to treat the constants as units that always need a coefficient for readability:
| DefaultNativeHistogramMinResetDuration = time.Hour | |
| DefaultNativeHistogramMinResetDuration = 1 * time.Hour |
4 participants
💸 TL;DR
Add support for native histograms using the following settings:
Jira
This changes means that all histograms defined in baseplate.go will now be scraped as native histograms if the service is scraped using the
PrometheusProtoprotocol. This also requires the resulting prometheus instance to be configured to accept native histograms.If the service is scraped using plantext (the default), then the existing classic histograms will be scraped and this change doesn't change anything for the resulting metrics.
NB:
🧪 Testing Steps / Validation
Ran in snoodev, made sure that when namespace was configured to scrape native histograms, native histograms would show up, and classic histograms would still appear if prometheus was not configured to scrape native histograms.
✅ Checks