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

ingester: ActiveSeriesRequest can return native histograms information #7986

Merged
merged 6 commits into from
May 3, 2024

Conversation

krajorama
Copy link
Contributor

@krajorama krajorama commented Apr 26, 2024

What this PR does

ingester: ActiveSeriesRequest can return native histograms information

The interface ActiveSeries(ActiveSeriesRequest), gets a new Type field that requests active series by default (0) but can request native histograms (1). In case of histograms the new bucket_count array is filled with bucket counts in the response. Both these changes should be backwards compatible, enabling a smooth upgrade.

Internally I flipped the order of postings iterators from sharding over active series to filtering active native histogram series over sharding to be able to get the bucket counts from the postings iterator directly. Also removes the use of the intermediate postings list to be able to directly stream the results.

Two alternative were considered:

  1. add a new RPC interface. Lot's of duplicate code.
  2. encode bucket count in a special label. Due to tag=stringlabels this is not trivial and also would involve extra processing to put the new label into its properly sorted position in the labels. Overall sounds messy.

Which issue(s) this PR fixes or relates to

Related to #7981

Checklist

  • Tests updated.
  • N/A Documentation added.
  • N/A deferred CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • N/A about-versioning.md updated with experimental features.

@krajorama krajorama requested a review from a team as a code owner April 26, 2024 13:02
@krajorama krajorama force-pushed the krajo/native-histograms-cardinality-api branch 2 times, most recently from b8fdaa7 to 1b4938c Compare April 26, 2024 13:22
@krajorama krajorama marked this pull request as draft April 29, 2024 07:38
@krajorama krajorama force-pushed the krajo/native-histograms-cardinality-api branch from 1b4938c to 7660981 Compare April 29, 2024 08:32
@krajorama krajorama changed the title ingester: add RPC interface to fetch active native histogram information ingester: ActiveSeriesRequest can return native histograms information Apr 29, 2024
@krajorama krajorama force-pushed the krajo/native-histograms-cardinality-api branch from 7660981 to 8539e11 Compare April 29, 2024 08:43
@krajorama krajorama marked this pull request as ready for review April 29, 2024 08:44
Allow ActiveSeriesRequest to return only native histograms and
include active bucket count in the label __nh_bucket_count__

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
flxbk
flxbk previously approved these changes Apr 29, 2024
Copy link
Contributor

@flxbk flxbk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Do you have benchmark results to post by any chance?

@krajorama

This comment was marked as outdated.

@krajorama krajorama force-pushed the krajo/native-histograms-cardinality-api branch from 8539e11 to e19a6b2 Compare April 30, 2024 07:26
Labels are already packed due to tag==stringlabels in TSDB so Add()
doesn't work on them. And after they are decoded for the response we'd
have to insert the bucket count label into the correctly sorted position
which would mean never being able to just send the encoded labels.
Instead of adding a new interface I'll add a backwards compatible array
of integers as a compromise.

Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
@krajorama krajorama requested a review from flxbk May 1, 2024 14:03
@krajorama krajorama dismissed flxbk’s stale review May 1, 2024 14:03

I had to change the implementation to account for tag=stringlabels

@krajorama
Copy link
Contributor Author

$ benchstat old.txt new.txt 
goos: linux
goarch: amd64
pkg: github.com/grafana/mimir/pkg/ingester
cpu: AMD Ryzen 7 4700U with Radeon Graphics         
                                       │   old.txt    │               new.txt               │
                                       │    sec/op    │   sec/op     vs base                │
Ingester_ActiveSeries/few_series-8       4.824µ ±  6%   4.727µ ± 1%   -2.01% (p=0.007 n=10)
Ingester_ActiveSeries/~10%_of_series-8   2.506m ± 12%   2.222m ± 1%  -11.36% (p=0.005 n=10)
geomean                                  110.0µ         102.5µ        -6.80%

                                       │   old.txt    │               new.txt               │
                                       │     B/op     │     B/op      vs base               │
Ingester_ActiveSeries/few_series-8       2.461Ki ± 0%   2.430Ki ± 0%  -1.27% (p=0.000 n=10)
Ingester_ActiveSeries/~10%_of_series-8   618.4Ki ± 0%   618.3Ki ± 0%  -0.01% (p=0.000 n=10)
geomean                                  39.01Ki        38.76Ki       -0.64%

                                       │   old.txt   │               new.txt                │
                                       │  allocs/op  │  allocs/op   vs base                 │
Ingester_ActiveSeries/few_series-8        48.00 ± 0%    48.00 ± 0%       ~ (p=1.000 n=10) ¹
Ingester_ActiveSeries/~10%_of_series-8   8.437k ± 0%   8.437k ± 0%       ~ (p=1.000 n=10) ¹
geomean                                   636.4         636.4       +0.00%
¹ all samples are equal

Copy link
Contributor

@flxbk flxbk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me with some small nits.

pkg/ingester/client/ingester.proto Show resolved Hide resolved
pkg/ingester/active_series_test.go Outdated Show resolved Hide resolved
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Copy link
Contributor

@flxbk flxbk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@krajorama krajorama merged commit e7b53e7 into main May 3, 2024
29 checks passed
@krajorama krajorama deleted the krajo/native-histograms-cardinality-api branch May 3, 2024 09:52
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.

None yet

2 participants