-
Notifications
You must be signed in to change notification settings - Fork 719
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
*: add region heartbeat duration breakdown metrics #7871
Conversation
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #7871 +/- ##
==========================================
+ Coverage 73.32% 73.49% +0.17%
==========================================
Files 435 435
Lines 48195 48225 +30
==========================================
+ Hits 35337 35442 +105
+ Misses 9784 9728 -56
+ Partials 3074 3055 -19
Flags with carried forward coverage won't be shown. Click here to find out more. |
23be40e
to
e36b9c1
Compare
bd05e2d
to
25e3e45
Compare
Signed-off-by: nolouch <[email protected]>
25e3e45
to
ecf93b6
Compare
7a5d15a
to
509e5d9
Compare
preCheckDurationSum.Add(h.preCheckDuration.Seconds()) | ||
preCheckCount.Inc() |
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.
Is it possible to flush these metrics asynchronously? It appears that setting the metrics in the middle of processing may affect the performance.
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 think it's ok because all of them use atomic. and we can dynamically disable the trace by the config enable-heartbeat-breakdown-metrics
Signed-off-by: nolouch <[email protected]>
Signed-off-by: nolouch <[email protected]>
a17a5cf
to
4d9a680
Compare
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.
BTW, will it cause too much lock contention?
pkg/core/region.go
Outdated
@@ -1653,6 +1695,42 @@ func (r *RegionsInfo) GetRegionSizeByRange(startKey, endKey []byte) int64 { | |||
return size | |||
} | |||
|
|||
// metrics default poll interval | |||
const magicCount = 15 * time.Second |
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.
How about just DefaultPollInterval?
Signed-off-by: nolouch <[email protected]>
@rleungx This PR does not introduce additional locking and unlocking processes. All trace operations are added within the existing locks. Since metrics are directly atomic operations, I think it's okay, but it may increase some CPU time (holding locks). However, this CPU time should be relatively small compared to other parts. |
ptal @JmPotato |
Should we conduct a benchmark test on this feature to assess any potential impact on performance? |
@JmPotato Here is a benchmark result with the follower code :
ResultNoop tracer
Metrics Tracer
|
already approved by @easonn7 /merge |
@nolouch: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
This pull request has been accepted and is ready to merge. Commit hash: 1bf72a3
|
why not add metrics in |
What problem does this PR solve?
Issue Number: Close #7868
What is changed and how does it work?
Check List
Tests
Code changes
Side effects
Related changes
pingcap/docs
/pingcap/docs-cn
:pingcap/tiup
:Release note