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

refactor(metrics): use exponential_buckets #2101

Merged
merged 8 commits into from
Apr 25, 2022
Merged

Conversation

Sunt-ing
Copy link
Contributor

@Sunt-ing Sunt-ing commented Apr 25, 2022

What's changed and what's your intention?

I refactored all Histogram buckets from DEFAULT_BUCKETS+scale or manually setting every bucket to exponential_buckets.

Reasons:

  • Parameters in exponential_buckets are simple, straightforward, but powerful. In exponential_buckets, we can adjust three parameters: start, factor, bucket_count.
    • Compare to using DEFAULT_BUCKETS+scale, it's more flexible.
    • Compare to using manually setting every bucket, it's easier to tune or understand.
  • TiKV also uses this method.

Currently, the parameters used in this PR are set according to the previous parameters and my experience.

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests

Refer to a related PR or issue link (optional)

close #1006

@codecov
Copy link

codecov bot commented Apr 25, 2022

Codecov Report

Merging #2101 (95c9d09) into main (b986bfa) will increase coverage by 0.00%.
The diff coverage is 80.48%.

@@           Coverage Diff            @@
##             main    #2101    +/-   ##
========================================
  Coverage   70.82%   70.83%            
========================================
  Files         639      640     +1     
  Lines       81173    81485   +312     
========================================
+ Hits        57494    57717   +223     
- Misses      23679    23768    +89     
Flag Coverage Δ
rust 70.83% <80.48%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/storage/src/monitor/hummock_metrics.rs 0.00% <0.00%> (ø)
src/batch/src/executor/monitor/stats.rs 100.00% <100.00%> (ø)
src/meta/src/rpc/metrics.rs 78.70% <100.00%> (ø)
src/storage/src/monitor/state_store_metrics.rs 99.51% <100.00%> (-0.06%) ⬇️
src/storage/src/hummock/block_cache.rs 77.27% <0.00%> (-10.97%) ⬇️
src/storage/src/object/mod.rs 21.62% <0.00%> (-10.82%) ⬇️
src/storage/src/hummock/sstable_store.rs 64.17% <0.00%> (-9.31%) ⬇️
...rc/frontend/src/optimizer/rule/index_delta_join.rs 94.36% <0.00%> (-5.64%) ⬇️
src/storage/src/hummock/compactor.rs 70.33% <0.00%> (-1.81%) ⬇️
src/storage/src/object/mem.rs 80.64% <0.00%> (-1.08%) ⬇️
... and 29 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@Sunt-ing Sunt-ing marked this pull request as ready for review April 25, 2022 04:31
@Sunt-ing Sunt-ing requested a review from twocode April 25, 2022 07:05
@Sunt-ing
Copy link
Contributor Author

Changed. @Little-Wallace

"Histogram of time spent from compacting shared buffer to remote storage.",
buckets
"Histogram of time spent from compacting shared buffer to remote storage",
exponential_buckets(0.0001, 2.0, 16).unwrap() // max 3s
Copy link
Contributor

Choose a reason for hiding this comment

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

3s is too small.
we can start since 1ms and we can observe 64s at most

let opts = histogram_opts!(
"state_store_write_build_l0_sst_duration",
"Total time of batch_write_build_table that have been issued to state store",
buckets
exponential_buckets(0.001, 2.0, 16).unwrap() // max 32s
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest start from 1ms because most

Copy link
Contributor

@Little-Wallace Little-Wallace left a comment

Choose a reason for hiding this comment

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

LGTM

@Sunt-ing Sunt-ing enabled auto-merge (squash) April 25, 2022 09:17
@Sunt-ing Sunt-ing merged commit 74b04e5 into main Apr 25, 2022
@Sunt-ing Sunt-ing deleted the sunt_calibrate_params branch April 25, 2022 09:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calibrate the histogram buckets and make them configurable.
2 participants