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 billing metrics to ingester #5318

Merged
merged 9 commits into from
Jul 11, 2023

Conversation

zenador
Copy link
Contributor

@zenador zenador commented Jun 23, 2023

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 and cortex_ingester_active_series_custom_tracker are supersets of cortex_ingester_active_native_histogram_series and cortex_ingester_active_native_histogram_series_custom_tracker respectively. cortex_ingester_active_native_histogram_buckets and cortex_ingester_active_native_histogram_buckets_custom_tracker are the bucket equivalents.

Which issue(s) this PR fixes or relates to

Fixes: #5365

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@zenador zenador requested a review from a team as a code owner June 23, 2023 08:34
@zenador zenador requested a review from krajorama June 23, 2023 08:35
pkg/ingester/activeseries/active_series.go Outdated Show resolved Hide resolved
pkg/ingester/activeseries/active_series.go Outdated Show resolved Hide resolved
pkg/ingester/activeseries/active_series_test.go Outdated Show resolved Hide resolved
pkg/ingester/ingester.go Outdated Show resolved Hide resolved
pkg/ingester/ingester.go Show resolved Hide resolved
pkg/ingester/ingester.go Outdated Show resolved Hide resolved
}

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()
Copy link
Contributor

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.

@zenador zenador force-pushed the zenador/add-histogram-billing-metrics-ingester branch from 7b84b4e to e288246 Compare June 29, 2023 05:18
@krajorama
Copy link
Contributor

Please add CHANGELOG entry that lists the additional metrics and their meaning.

Comment on lines 271 to 286
for i := 0; i < matchesLen; i++ {
s.activeMatchingNativeHistogramBuckets[matches.get(i)] += diff
}
Copy link
Contributor

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.

@zenador zenador force-pushed the zenador/add-histogram-billing-metrics-ingester branch 2 times, most recently from 47c2523 to e5901e8 Compare July 6, 2023 20:15
@zenador zenador force-pushed the zenador/add-histogram-billing-metrics-ingester branch from e5901e8 to b2b60ab Compare July 7, 2023 07:42
Copy link
Contributor

@krajorama krajorama left a 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

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/ingester/activeseries/active_series.go Outdated Show resolved Hide resolved
pkg/ingester/activeseries/active_series.go Show resolved Hide resolved
Copy link
Contributor

@charleskorn charleskorn left a 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 <[email protected]>
@charleskorn charleskorn merged commit a9f4bdf into main Jul 11, 2023
28 checks passed
@charleskorn charleskorn deleted the zenador/add-histogram-billing-metrics-ingester branch July 11, 2023 01:18
@bboreham
Copy link
Contributor

bboreham commented Aug 8, 2023

This is rather expensive - do we really need the detail per-tracker?
(4 bytes per count * 2 new counts * 128 slices * number of trackers * number of tenants)

(Even worse before #5665)

@zenador
Copy link
Contributor Author

zenador commented Aug 8, 2023

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.

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.

Active series should track native histograms and buckets
4 participants