fix: Use prometheus summary for high watermark metric - #21906
Conversation
grobinson-grafana
changed the title
![cursor[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
5703baa to
26b85f5
Compare
| bufferedBytes: promauto.With(reg).NewSummary(prometheus.SummaryOpts{ | ||
| Name: "pattern_ingester_tee_buffered_bytes", | ||
| Help: "The current number of bytes buffered in the tee.", | ||
| Objectives: map[float64]float64{1.0: 0.1}, |
There was a problem hiding this comment.
We only care about the maximum value, hence 1.0.
joe-elliott
left a comment
joe-elliott
left a comment
There was a problem hiding this comment.
Shouldn't we also remove:
https://github.com/grafana/loki/blob/main/pkg/util/metric/max_sample_collector.go
?
| // It is safe for concurrent use. | ||
| func (ts *TeeService) releaseBufferedBytes(size int) { | ||
| newVal := ts.bufferedBytes.Add(-int64(size)) | ||
| ts.metrics.bufferedBytes.Observe(float64(newVal)) |
There was a problem hiding this comment.
I don't believe we need to update the summary in releaseBufferedBytes, that will be taken care of the next time someone calls reserveBufferedBytes.
| // the same value. | ||
| // | ||
| // It is safe for concurrent use. | ||
| func (ts *TeeService) reserveBufferedBytes(size int) bool { |
There was a problem hiding this comment.
i don't think we should make the change here, but would this make sense to be a semaphore?
There was a problem hiding this comment.
There are a few differences in that x/sync/semaphore uses mutexes instead of atomics, and I'm not sure we need a waiters list with first-come first-serve semantics. I'd be curious to experiment with it if it has benefits.
2 participants
What this PR does / why we need it:
Having discussed with various Grafanaistas, this seems preferred to a high watermark gauge.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Checklist
CONTRIBUTING.mdguide (required)docs/sources/setup/upgrade/_index.mddeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PRNote
Medium Risk
Changes the
pattern_ingester_tee_buffered_bytesmetric type/semantics and updates buffering accounting paths, which can affect dashboards/alerts and adds per-update Summary observations on hot paths.Overview
Switches
pattern_ingester_tee_buffered_bytesfrom a custom max-sample collector to a PrometheusSummary, recording the buffered-bytes value viaObserve()on each reservation/release and removing the prior start/stop lifecycle registration.Refactors
tryReserveBufferedBytesintoreserveBufferedBytesso buffered bytes are always tracked while only enforcingMaxBufferedByteswhen it’s positive, and updatesTestPatternTee_MaxBufferedBytesto reflect the new “limit disabled” behavior.Reviewed by Cursor Bugbot for commit 26b85f5. Bugbot is set up for automated code reviews on this repo. Configure here.