-
Notifications
You must be signed in to change notification settings - Fork 2.2k
[BugFix] Fix merge commit latency metrics overflow #67168
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
Conversation
Signed-off-by: PengFei Li <[email protected]>
Signed-off-by: PengFei Li <[email protected]>
5f0a6d6 to
041ac9d
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.
Pull request overview
This PR fixes an overflow issue in merge commit latency metrics by converting the unit from nanoseconds to microseconds. The bvar LatencyRecorder uses an IntRecorder internally which can only store values up to ~2.1 seconds when using nanoseconds, causing overflows for longer operations. By switching to microseconds, the maximum supported latency increases to ~35.8 minutes.
Key changes:
- Changed metric unit from nanoseconds to microseconds with a NS_TO_US macro conversion
- Renamed all latency recorder variables from
_nssuffix to_ussuffix - Updated documentation in English, Chinese, and Japanese to reflect the unit change
- Fixed a typo in metric name from "reqeust" to "request"
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| be/src/runtime/batch_write/isomorphic_batch_write.cpp | Updated bvar latency recorders to use microseconds, added NS_TO_US conversion macro, and applied conversions throughout the code |
| docs/en/administration/management/monitoring/metrics.md | Added documentation for merge commit metrics with microsecond units and version note |
| docs/zh/administration/management/monitoring/metrics.md | Added Chinese documentation for merge commit metrics with microsecond units and version note |
| docs/ja/administration/management/monitoring/metrics.md | Added Japanese documentation for merge commit metrics with microsecond units and version note |
Signed-off-by: PengFei Li <[email protected]>
|
@cursor review |
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.
✅ Bugbot reviewed your changes and found no bugs!
Signed-off-by: PengFei Li <[email protected]>
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[FE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[BE Incremental Coverage Report]✅ pass : 21 / 22 (95.45%) file detail
|
|
@cursor review |
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.
✅ Bugbot reviewed your changes and found no bugs!
|
@Mergifyio backport branch-3.4 |
|
@Mergifyio backport branch-3.5 |
|
@Mergifyio backport branch-4.0 |
✅ Backports have been createdDetails
|
✅ Backports have been createdDetails
|
✅ Backports have been createdDetails
|
Signed-off-by: PengFei Li <[email protected]> (cherry picked from commit 39bc4ea) # Conflicts: # docs/en/administration/management/monitoring/metrics.md # docs/ja/administration/management/monitoring/metrics.md # docs/zh/administration/management/monitoring/metrics.md
Signed-off-by: PengFei Li <[email protected]> (cherry picked from commit 39bc4ea)
Signed-off-by: PengFei Li <[email protected]> (cherry picked from commit 39bc4ea)
…67275) Signed-off-by: PengFei Li <[email protected]> Co-authored-by: PengFei Li <[email protected]>
…67274) Signed-off-by: PengFei Li <[email protected]> Co-authored-by: PengFei Li <[email protected]>
Signed-off-by: PengFei Li <[email protected]>
Why I'm doing:
Merge commit latency metrics use bvar
LatencyRecorder, and the unit is nanoseconds, but if the latency is larger than 2 seconds, the bvar will report overflow as the followingThe reason is that although
LatencyRecorderacceptsint64_tas input (https://github.com/apache/brpc/blob/master/src/bvar/latency_recorder.h#L100), but it actually uses
IntRecorderto store the latency as int (https://github.com/apache/brpc/blob/master/src/bvar/latency_recorder.h#L54), so the allowed latency in nanosecond is2147483647(about 2.1s)What I'm doing:
bvar does not provide other recorders to support
int64_t. For merge commit latency, it's not necessary to use nanosecond precision, so just change the latency unit to microsecond, and the max latency can be 35.8 minutes which should be enough.Note changing metric unit introduces behavior change, and add the document to explain it
Fixes #issue
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check:
Note
Fixes overflow in merge-commit latency metrics by switching from nanoseconds to microseconds and documenting the change.
g_mc_*_latency_nswithg_mc_*_latency_usinisomorphic_batch_write.cpp, addNS_TO_USconversions, and update logs/recorders accordinglyWritten by Cursor Bugbot for commit 126d8f7. This will update automatically on new commits. Configure here.