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

Store: Add a few objectives for Store's data touched/fetched amount and sizes #5819

Conversation

douglascamata
Copy link
Contributor

@douglascamata douglascamata commented Oct 25, 2022

Signed-off-by: Douglas Camata 159076+douglascamata@users.noreply.github.com

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

  • Add a few objectives for the summaries reporting the quantity and size of items fetched per data type from object storage & cache. They are: 50, 95, and 99 quantiles.

The intent is to provide some metrics that will help inform users when setting values for the flags store.grpc.series-sample-limit and store.grpc.touched-series-limit.

Verification

  • Ran it locally and confirmed the metrics are there and producing the configured quantiles.

@douglascamata douglascamata changed the title Store: Add a few objectives for Store's data touched/fetched Store: Add a few objectives for Store's data touched/fetched amount and sizes Oct 25, 2022
The intent is to provide some metrics that will help inform users when setting values for the flags `store.grpc.series-sample-limit` and `store.grpc.touched-series-limit`.

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
@douglascamata douglascamata force-pushed the bucket-store-data-touched-fetched-objectives branch from 589a285 to cbf3484 Compare October 25, 2022 15:27
Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
@douglascamata douglascamata force-pushed the bucket-store-data-touched-fetched-objectives branch from cbf3484 to a4cea43 Compare October 25, 2022 15:28
@douglascamata douglascamata marked this pull request as ready for review October 26, 2022 12:25
pkg/store/bucket.go Outdated Show resolved Hide resolved
pkg/store/bucket.go Outdated Show resolved Hide resolved
pkg/store/bucket.go Show resolved Hide resolved
pkg/store/bucket.go Show resolved Hide resolved
pkg/store/bucket.go Show resolved Hide resolved
pkg/store/bucket.go Outdated Show resolved Hide resolved
pkg/store/bucket.go Show resolved Hide resolved
pkg/store/bucket.go Outdated Show resolved Hide resolved
pkg/store/bucket.go Outdated Show resolved Hide resolved
@douglascamata
Copy link
Contributor Author

@moadz I would love to simplify and make the metric names shorter, but I don't think it's a good idea to change their names after they have been out since many releases. The reason is that people might be already relying on them for dashboards (i.e. kube-thanos uses them) and alerts.

douglascamata and others added 2 commits October 26, 2022 16:39
Co-authored-by: Moad Zardab <zardab12@hotmail.com>
Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
moadz
moadz previously approved these changes Oct 26, 2022
Copy link
Contributor

@moadz moadz left a comment

Choose a reason for hiding this comment

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

@douglascamata you're absolutely right. I misread the diff and thought you were adding them as new metrics :) Bucket changes LGTM

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
Copy link
Contributor

@philipgough philipgough left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

@bwplotka bwplotka merged commit e46af28 into thanos-io:main Nov 23, 2022
@@ -34,6 +34,7 @@ We use *breaking :warning:* to mark changes that are not backward compatible (re
### Removed

- [#5824](https://github.com/thanos-io/thanos/pull/5824) Mixin: Remove noisy `ThanosReceiveTrafficBelowThreshold` alert.
- [#5819](https://github.com/thanos-io/thanos/pull/5819) Store: Add a few objectives for Store's data touched/fetched amount and sizes. They are: 50, 95, and 99 quantiles.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this was accidentally put into removed section instead of added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, my bad, @matej-g. Fixed in #5919.

ngraham20 pushed a commit to ngraham20/thanos that referenced this pull request May 18, 2023
…nd sizes (thanos-io#5819)

* Add a few objectives for Store's data touched/fetched

The intent is to provide some metrics that will help inform users when setting values for the flags `store.grpc.series-sample-limit` and `store.grpc.touched-series-limit`.

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

* Update CHANGELOG

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

* Update some metric's help text

Co-authored-by: Moad Zardab <zardab12@hotmail.com>
Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

* Remove quantiles from redundant metric

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

* Fix spelling

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

* Fix some metric help texts

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>

Signed-off-by: Douglas Camata <159076+douglascamata@users.noreply.github.com>
Co-authored-by: Moad Zardab <zardab12@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants