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: Expose bucket index operation duration histogram #2725

Merged
merged 7 commits into from
Aug 10, 2020

Conversation

kakkoyun
Copy link
Member

@kakkoyun kakkoyun commented Jun 5, 2020

I would like to expose bucket fetch operation durations to fine-tune caching layer for store.

Signed-off-by: Kemal Akkoyun kakkoyun@gmail.com

  • I added CHANGELOG entry for this change.

Changes

  • Add metrics to expose already collected fetch durations.

Verification

  • make build

@kakkoyun kakkoyun requested a review from bwplotka June 5, 2020 06:41
pkg/store/bucket.go Outdated Show resolved Hide resolved
pkg/store/bucket.go Outdated Show resolved Hide resolved
pkg/store/bucket.go Outdated Show resolved Hide resolved
@kakkoyun
Copy link
Member Author

kakkoyun commented Jun 5, 2020

@pracucci Thanks a lot for correcting my sloppy work 🙈 I haven't finished my morning coffee yet.

Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Thanks @kakkoyun. I see a value in exposing these timings, but I'm not sure the way we track them right now is valuable. Please see the comment I left.

pkg/store/bucket.go Outdated Show resolved Hide resolved
@kakkoyun kakkoyun marked this pull request as draft June 5, 2020 06:55
pkg/store/bucket.go Outdated Show resolved Hide resolved
@kakkoyun kakkoyun changed the title store: Expose bucket fetch operation duration histograms [WIP] store: Expose bucket fetch operation duration histograms Jul 21, 2020
kakkoyun and others added 4 commits August 5, 2020 14:48
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Co-authored-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun kakkoyun changed the title [WIP] store: Expose bucket fetch operation duration histograms [WIP] store: Expose bucket index operation duration histogram Aug 6, 2020
@kakkoyun kakkoyun marked this pull request as ready for review August 6, 2020 10:01
@kakkoyun kakkoyun changed the title [WIP] store: Expose bucket index operation duration histogram store: Expose bucket index operation duration histogram Aug 6, 2020
pkg/store/bucket.go Outdated Show resolved Hide resolved
pkg/store/bucket.go Outdated Show resolved Hide resolved
pkg/store/bucket.go Outdated Show resolved Hide resolved
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
pkg/store/bucket.go Outdated Show resolved Hide resolved
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
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.

Amazing, just some nits (:

Buckets: []float64{0.001, 0.01, 0.1, 0.3, 0.6, 1, 3, 6, 9, 20, 30, 60, 90, 120},
})

m.postingsLookupDuration = promauto.With(reg).NewHistogram(prometheus.HistogramOpts{
Copy link
Member

Choose a reason for hiding this comment

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

Can we have just label? It will much easier to aggregate, plus we already do that for other statistics, WDYT?

I think also bucket can be adjusted for timeout. BTW What lookup means is quite vague by this help 🤔 Maybe we can be explicit more.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bwplotka The first version was using a single histogram with labels 8a46f33. Then I decided to change it, just to be consistent with what we already have in here.

BTW What lookup means is quite vague by this help 🤔 Maybe we can be explicit more.

I'm gonna change the name as you suggested and try to clarify the description.

I think also bucket can be adjusted for timeout.

I've put the boundary at the query timeout. Which timeout value is used for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

@bwplotka friendly ping.

Copy link
Member

Choose a reason for hiding this comment

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

ok for now 👍

})

m.postingsLookupDuration = promauto.With(reg).NewHistogram(prometheus.HistogramOpts{
Name: "thanos_bucket_store_index_postings_lookup_duration_seconds",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Name: "thanos_bucket_store_index_postings_lookup_duration_seconds",
Name: "thanos_bucket_store_index_cachable_fetches_duration_seconds",

I would add cachable to put highlight that it includes cache (:

Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Thanks @kakkoyun for addressing my feedback. Latest changes 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.

Thanks!

LGTM 💪

Buckets: []float64{0.001, 0.01, 0.1, 0.3, 0.6, 1, 3, 6, 9, 20, 30, 60, 90, 120},
})

m.postingsLookupDuration = promauto.With(reg).NewHistogram(prometheus.HistogramOpts{
Copy link
Member

Choose a reason for hiding this comment

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

ok for now 👍

Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun kakkoyun merged commit dec6c09 into thanos-io:master Aug 10, 2020
@kakkoyun kakkoyun deleted the bucket_fetch_metrics branch August 10, 2020 12:36
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.

3 participants