Begin implementation for native histograms - #3789
Conversation
2f53a6d to
490b7b4
Compare
9d33bdf to
1bed650
Compare
joe-elliott
left a comment
joe-elliott
left a comment
There was a problem hiding this comment.
looks good. some comments to work through
|
|
||
| func (h *nativeHistogram) activeSeriesPerHistogramSerie() uint32 { | ||
| // TODO can we estimate this? | ||
| return 1 |
There was a problem hiding this comment.
this is interesting. i suppose this plays into how many series prom/mimir consider a native histogram?
There was a problem hiding this comment.
Right, I think we have something to learn here, since there doesn't seem to be a clean mapping between native histograms and active series.
| if err != nil { | ||
| return activeSeries, err | ||
| } | ||
| s.histogram = encodedMetric.GetHistogram() |
There was a problem hiding this comment.
is there a reason to store this as a field on the struct? it seems like we only use it in this method. if that's true we should remove it from the struct
There was a problem hiding this comment.
This was added so that we could remember which histograms were sent. The Reset call below captures this, and storing the state on the struct was a way for us to get around this issue. If we don't know which buckets have had their exemplars reported, they get updated with the most recent timestamp and sent again, since the series keeps the exemplars.
There was a problem hiding this comment.
yeah, i'm not seeing this. it seems only referenced in this method which makes me think we should just use a local var. perhaps i'm missing something. we can sync on this.
| // NOTE: A subtle point here is that the new histogram implementation is | ||
| // chosen in the registry. This means that if the user overrides change | ||
| // from "both" to "classic", we will still be using the new histograms | ||
| // implementation. This is because the registry is not recreated when the | ||
| // overrides change. This is a tradeoff to avoid recreating the registry | ||
| // for every change in the overrides, but it does mean that even if user | ||
| // changes to "classic" histograms, we will still be using the new | ||
| // histograms implementation and want to avoid generating native | ||
| // histograms. |
There was a problem hiding this comment.
This will work when we switch from native or both to classic, but it will not work when we go from classic to anything else. If we initiate their tenant with classic we use the old histogram implementation, when we change the override nothing will be updated and we keep using this implementation.
To workaround this we need to recreate the processors I think so they all re-register their histograms.
But maybe something quick to fix in a follow-up PR?
There was a problem hiding this comment.
Right, I think let's follow up.
* Initial setup native histograms * Bump prometheus dependencies, map exemplars over * Add GenerateNativeHistograms from legacy * Add test coverage for legacy overrides * Plumb overrides into remote write config generation and test * Lint for unused vars and duplicate imports * Map 'classic' histograms out of prom.Histogram * More tweaking to get classic histograms working, not there yet though :( * Lint increment * Lint increment * Refactor native histogram tests * Track and reset the buckets for which exemplars have been recorded * Split multiplier test into integer and floating point * Fix expectedSeriesCount and examplars in test * Drop expectedSeriesCount and just len of expectedSamples instead * Reduce log spam * Apply existing interface constraint to Gauge and Counter * Update test instances for metric interface * Use int64 in place of atomic since always under lock * Lint for interface updates * Drop series[0] check * Convert override from bool to string and test values * Push mode func into native histograms implementation for update handling * Drop unused variable * Set --enable-feature=native-histograms on all prometheus docker-compose setups * Update generate_native_histograms setting --------- Co-authored-by: Koenraad Verheyden <koenraad.verheyden@grafana.com> Co-authored-by: yuna <yuna.verheyden@posteo.net>
3 participants
What this PR does:
Here we add what is necessary to generate and remote-write native histograms from the metrics-generator.
Which issue(s) this PR fixes:
Fixes #
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]