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

Improve index header metrics #6847

Merged
merged 3 commits into from
Oct 27, 2023
Merged

Conversation

harry671003
Copy link
Contributor

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

Changes

  • Add more histogram buckets to thanos_bucket_store_block_load_duration_seconds metrics.
  • Add indexheader_download_duration_seconds for tracking how long it takes to download the indexheader from objstore.
  • Add a new metrics indexheader_load_duration_seconds to track how long it takes to initialize the binary reader.

@harry671003 harry671003 changed the title Improve binary reader metrics Improve index header metrics Oct 25, 2023
@harry671003 harry671003 marked this pull request as ready for review October 25, 2023 22:43
yeya24
yeya24 previously approved these changes Oct 26, 2023
Copy link
Contributor

@yeya24 yeya24 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! Thanks

@@ -189,9 +189,8 @@ func newBucketStoreMetrics(reg prometheus.Registerer) *bucketStoreMetrics {
m.blockLoadDuration = promauto.With(reg).NewHistogram(prometheus.HistogramOpts{
Name: "thanos_bucket_store_block_load_duration_seconds",
Help: "The total time taken to load a block in seconds.",
Buckets: []float64{0.1, 0.5, 1, 10, 20, 30, 60, 120},
Buckets: []float64{0.01, 0.02, 0.05, 0.1, 0.2, 0.5, 1, 2, 5, 15, 30, 60, 90, 120, 300},
Copy link
Contributor

Choose a reason for hiding this comment

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

I am thinking about maybe we can start from a larger number like 1s instead of having multiple small buckets from 0.01 to 0.5.
WDYT? I feel like we don't care much about loading blocks fast. For example, we load block from disk cache.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same might apply to other two metrics you 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.

I agree. At least for the thanos_bucket_store_block_load_duration_seconds and the indexheader_download_duration_seconds, it may not be reasonable to expect it to take less than 100ms.

But the indexheader_load_duration_seconds should take much less right? I'll keep the smaller buckets on that metric.

yeya24
yeya24 previously approved these changes Oct 26, 2023
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks

Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Can you update changelog and mention the new metrics added?

Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com>
Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com>
@yeya24
Copy link
Contributor

yeya24 commented Oct 26, 2023

Need to sign off DCO

Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com>
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Thanks!

@yeya24 yeya24 merged commit 15fadd5 into thanos-io:main Oct 27, 2023
16 checks passed
douglascamata pushed a commit to douglascamata/thanos that referenced this pull request Dec 13, 2023
* Improve binary reader metrics

Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com>

* Update buckets

Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com>

* Update CHANGELOG

Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com>

---------

Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com>
douglascamata pushed a commit to douglascamata/thanos that referenced this pull request Dec 13, 2023
* Improve binary reader metrics

Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com>

* Update buckets

Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com>

* Update CHANGELOG

Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.com>

---------

Signed-off-by: 🌲 Harry 🌊 John 🏔 <johrry@amazon.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.

2 participants