kvserver: split upreplication from recovery metrics #119028
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
andrewbaptist
force-pushed
the
2024-02-09-snapshot-metrics
branch
from
c827ff0 to
83499f5
Compare
There was a problem hiding this comment.
The change looks good. Should we include the new metric in the DB console snapshot chart here?
Also consider adding a test case that tests up-replication snapshot bytes. It doesn't appear that we have any atm, the closest I could find was for x-locality.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @andrewbaptist)
andrewbaptist
added
GA-blocker
branch-release-24.1
andrewbaptist
force-pushed
the
2024-02-09-snapshot-metrics
branch
from
83499f5 to
9cffd2e
Compare
andrewbaptist
added
the
backport-24.1.x
|
I realized I hadn't finished this as I was clearing out my 24.1 tasks. I still didn't create a unit test, but I added it to the UI and run a manual test to validate it shows up correctly in the UI and is populated.
Metrics in general are a pain to test well in unit tests, although maybe we should think about how to make this easier. Its easy to see if they are "zero or non-zero" but harder to tell if they are right. |
Previously both raft recovery and upreplication snapshots were counted as recovery metrics. This PR splits them into two separate categories. Epic: none Fixes: cockroachdb#115729 Release note (ops change): Adds 2 new metrics range.snapshots.upreplication.rcvd-bytes and range.snapshots.upreplication.sent-bytes. It also changes the meaning of range.snapshots.recovery.rcvd-bytes and range.snapshots.recovery.sent-bytes to only include raft snapshots. Additionally it adds the new line to the "Snapshot Data Received" graph.
andrewbaptist
force-pushed
the
2024-02-09-snapshot-metrics
branch
from
9cffd2e to
def6033
Compare
|
@kvoli let me know if you will get a chance to review this. We can also decide to not backport this to 24.1 and just put it in master. Thanks! The only change of note is the addition of the pages to the UI. |
There was a problem hiding this comment.
Reviewed 4 of 5 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status:
|
bors r=kvoli TFTR! |
Previously both raft recovery and upreplication snapshots were counted as recovery metrics. This PR splits them into two separate categories.
Epic: none
Fixes: #115729
Release note (ops change): Adds 2 new metrics
range.snapshots.upreplication.rcvd-bytes and
range.snapshots.upreplication.sent-bytes. It also changes the meaning of range.snapshots.recovery.rcvd-bytes and
range.snapshots.recovery.sent-bytes to only include raft snapshots.