-
Notifications
You must be signed in to change notification settings - Fork 512
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
native histograms: experimental cardinality api #8008
native histograms: experimental cardinality api #8008
Conversation
8539e11
to
e19a6b2
Compare
220f25c
to
73ef049
Compare
73ef049
to
000c750
Compare
Regeression benchmark for querier
|
000c750
to
b0a0626
Compare
The new endpoint gives a list of metric names of active native histogram series with associated statistics about active bucket counts. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
b0a0626
to
815d55d
Compare
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
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.
I had a few minor questions.
pkg/distributor/distributor.go
Outdated
// For native histograms we are going to return metrics level information so we don't need to limit the response size. | ||
if !nativeHistograms { | ||
if limit := d.limits.ActiveSeriesResultsMaxSizeBytes(tenantID); limit > 0 { | ||
maxResponseSize = limit | ||
} |
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.
Why should we apply the limit selectively only? It's there to bound in-use memory per call and querier, if it is exceeded the request should be retried with a higher shard count. Maybe I'm missing something, but I don't see why the shape of the response data makes any difference?
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.
Yeah, I'm not sure about this one. My problem is the name of the limit , it's called querier.active-series-results-max-size-bytes
, but we'd be limiting the intermediate result size. Something more relevant could be querier.max-fetched-series-per-query
, but that's for queries which have very different semantics than active series requests.
I'll restore it, add a test and update the help text to try and make it clear how it's used.
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.
Done, CI running now
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.
From a user perspective, I'm not sure if the new help text is clearer. What exactly is your issue with
Maximum size of an active series request result shard in bytes
?
but we'd be limiting the intermediate result size
that's exactly what we want though to avoid high in-use space in queriers if the shard count is too low
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.
From a user perspective, I'm not sure if the new help text is clearer. What exactly is your issue with
Maximum size of an active series request result shard in bytes?
I think I just missed the word "shard" in it. I'll update to just add active native histogram metrics to the existing sentence.
// Cannot start streaming until we merged all results. | ||
err := g.Wait() |
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.
If we build the full response in memory then we don't need streaming at all, or am I missing something?
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.
True, but on the other hand stream writing JSON is more efficient as well as far as I know? So it made sense to adopt what worked already.
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.
I'm not sure about the efficiency. In general I think the non-streaming version is more robust because if there's any error during accumulation of the response the client will get a non-200 HTTP status code whereas with streaming the client will always get a 200 and if the response can't be built without error the stream will abort. Also, there would be less code complexity without the streaming. But since we've already got that complexity elsewhere maybe it's ok to duplicate it.
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.
Hmm, the error handling is more complicated in my case. That convinced me, I'll change to non streaming.
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.
Reworked it. I also switched from supporting "x-snappy-framed" to supporting "snappy" as encoding, since framing is for streaming and we don't need it here.
Co-authored-by: Felix Beuke <felix.j.beuke@gmail.com>
From review comment. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com>
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.
Thanks, lgtm
* query-frontend: add /api/v1/cardinality/active_native_histogram_metrics The new endpoint gives a list of metric names of active native histogram series with associated statistics about active bucket counts. Signed-off-by: György Krajcsovits <gyorgy.krajcsovits@grafana.com> Co-authored-by: Felix Beuke <felix.j.beuke@gmail.com>
What this PR does
Query-frontend, querier: new experimental
/cardinality/active_native_histogram_metrics
API to get active native histogram metric names with statistics about active native histogram buckets.Which issue(s) this PR fixes or relates to
Fixes #7981
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]
.about-versioning.md
updated with experimental features.