VDiff: Add some stats #15175
Merged
VDiff: Add some stats #15175
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Matt Lord <mattalord@gmail.com>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vitess-bot
bot
added
NeedsBackportReason
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #15175 +/- ##
==========================================
+ Coverage 67.29% 67.41% +0.12%
==========================================
Files 1560 1560
Lines 192089 192996 +907
==========================================
+ Hits 129259 130114 +855
- Misses 62830 62882 +52 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
mattlord
removed
the
NeedsBackportReason
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
mattlord
added
Type: Enhancement
Signed-off-by: Matt Lord <mattalord@gmail.com>
mattlord
requested review from
GuptaManan100,
rohit-nayak-ps and
shlomi-noach
as code owners
Signed-off-by: Matt Lord <mattalord@gmail.com>
mattlord
commented
Feb 10, 2024
| @@ -44,7 +44,10 @@ comment: # https://docs.codecov.com/docs/pull-request-comments | |||
|
|
|||
| coverage: | |||
| status: # https://docs.codecov.com/docs/commit-status | |||
| patch: | |||
| default: | |||
| informational: true # Don't ever fail the codecov/patch test | |||
There was a problem hiding this comment.
I meant to do this in: #14967
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Signed-off-by: Matt Lord <mattalord@gmail.com>
Because we're not deferring a new function, the parameters passed to Record are calculated when we register the defer and thus Record is able to compare the current time to the time we passed in when registering the defer (time.Now()). Signed-off-by: Matt Lord <mattalord@gmail.com>
rohit-nayak-ps
approved these changes
Feb 14, 2024
| defer testStats.controllers[id].TableDiffPhaseTimings.Record(phase, time.Now()) | ||
| time.Sleep(sleepTime) | ||
| } | ||
| want := int64(1.2 * float64(sleepTime)) // Allow 20% overhead for recording timing |
There was a problem hiding this comment.
We have seen flakiness with such precise timings in the past, especially since you are dealing in milliseconds here. We should probably just compare that it is not zero.
There was a problem hiding this comment.
I can update it if it becomes flaky. I don't recall ever seeing TestVReplicationStats fail, and that does the same thing. If they are flaky then we should update both.
Signed-off-by: Matt Lord <mattalord@gmail.com>
ajm188
approved these changes
Feb 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
VDiff emitted no metrics/stats (not technically true after #14786) so you could not monitor it in the standard Vitess ways — e.g. see some of the key VReplication stats. Instead you relied on the
vdiff showcommand. While theshowcommand provides the information you'd typically want to know about a specific vdiff — you could not monitor and graph vdiff stats over time and correlate those with other system events (e.g. to try and understand why a vdiff was taking longer than expected) and you got no aggregated stats across vdiffs.This PR adds some initial metrics that I believe will be helpful to monitor and debug the VDiff component and individual vdiffs over time. You can see example stats and metrics here: https://gist.github.com/mattlord/bf49025acc7fe0c9d84da35b86ccc555
Related Issue(s)
Checklist