-
Notifications
You must be signed in to change notification settings - Fork 532
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 billing metrics to ingester #5318
Add histogram billing metrics to ingester #5318
Conversation
} | ||
|
||
func (s *seriesStripe) findOrCreateEntryForSeries(ref uint64, series labels.Labels, nowNanos int64) (*atomic.Int64, bool) { | ||
func (s *seriesStripe) findAndUpdateOrCreateEntryForSeries(ref uint64, series labels.Labels, nowNanos int64, hasNativeHistograms bool, numNativeHistogramBuckets int) (*atomic.Int64, bool) { | ||
s.mu.Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO note: after this PR we should check how often the bucket count changes and whether we should merge findEntryForSeries and findAndUpdateOrCreateEntryForSeries to avoid double locking and refs lookup.
7b84b4e
to
e288246
Compare
Please add CHANGELOG entry that lists the additional metrics and their meaning. |
for i := 0; i < matchesLen; i++ { | ||
s.activeMatchingNativeHistogramBuckets[matches.get(i)] += diff | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: I've tried to come up with a way to make this go away, but couldn't. I guess it's fine since usually the matchesLen
will be 1 or a small number as you'd one to do charge-back to one time only.
47c2523
to
e5901e8
Compare
e5901e8
to
b2b60ab
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with a few comments on changelog and docstrings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo Krajo's suggestions.
Co-authored-by: George Krajcsovits <krajorama@users.noreply.github.com>
This is rather expensive - do we really need the detail per-tracker? (Even worse before #5665) |
Noted. As discussed on Slack, will check with @krajorama when he's back on whether we need this per-tracker or not. If not, we will remove it. If we do, we can have a separate set of trackers for native histograms so they won't take up extra space when unused. |
What this PR does
Add new metrics to ingester to track histogram series for billing, to count the number of active series that are native histograms, and the total number of buckets in those.
Does not modify existing metrics.
cortex_ingester_active_series
andcortex_ingester_active_series_custom_tracker
are supersets ofcortex_ingester_active_native_histogram_series
andcortex_ingester_active_native_histogram_series_custom_tracker
respectively.cortex_ingester_active_native_histogram_buckets
andcortex_ingester_active_native_histogram_buckets_custom_tracker
are the bucket equivalents.Which issue(s) this PR fixes or relates to
Fixes: #5365
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]