kv/kvclient: add distinct metric for each RPC method#46747
Merged
craig[bot] merged 3 commits intocockroachdb:masterfrom Jun 27, 2020
Merged
kv/kvclient: add distinct metric for each RPC method#46747craig[bot] merged 3 commits intocockroachdb:masterfrom
craig[bot] merged 3 commits intocockroachdb:masterfrom
Conversation
Member
|
This change is |
Contributor
Author
|
These metrics already paid dividends in #46752. |
Contributor
|
FWIW I'm a big fan of this. I'm also interested in adding this sort of counter on the server side. |
This was introduced all the way back in a1e027a, but was never directly tested. Release justification: testing only
This commit extends `AddMetricStruct` to process array types and add all non-nil metrics in arrays to its registry. This allows us to address a long-standing TODO where we used an array to point to distinct fields in a MetricStruct. We couldn't remove the fields because the array was ignored. With this change, we're now able to address the TODO. This commit sets up the next. Release justification: metric change that only impacts initialization-time logic.
6436f2c to
63fd40e
Compare
This commit adds a new counter metric for each of the 44 request methods. It then updates DistSender to track the use of these methods in their associated counters. The goal of this is to provide observability into the types of RPCs that are being sent in a cluster. We've seen a few cases in the past month where a cluster was sending more local and remote RPCs than expected, and being able to easily see what operations these RPCs were performing would have been a significant help to debugging. Release note (admin ui change): A collection of new metrics were added that track the method of requests that are sent as RPCs between nodes in a cluster. These metrics are named `distsender.rpc.<method>.sent`, with examples of `<method>` being "scan" and "conditionalput".
63fd40e to
10448db
Compare
Contributor
Author
Agreed. I'm surprised we have absolutely nothing for this. I opened #50660 to track this. I've rebased this and removed the WIP label, since I'm also more convinced something like this should exist. PTAL! |
andreimatei
reviewed
Jun 26, 2020
Contributor
andreimatei
left a comment
There was a problem hiding this comment.
LGTM
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @andreimatei)
ajwerner
reviewed
Jun 26, 2020
Contributor
ajwerner
left a comment
There was a problem hiding this comment.
Reviewable status:
Contributor
Author
|
TFTRs! bors r+ |
Contributor
Build succeeded |
miretskiy
pushed a commit
to miretskiy/cockroach
that referenced
this pull request
Sep 7, 2023
Extend metric registry to allow metric slices to be used. Prior to this change metric registry only alowed arrays. Slices are treated identical to the arrays -- the values are expanded, and each metric added to the registry. The array itself was not added. From some historical archeological perspective, cockroachdb#46747 added support for embedded metric Arrays. My best read on this PR is that it narrowly addressed the existing "TODOs", and avoided doing anything extra (which is good!). But it does not appear that there is a fundamental reason why slices should not be supported the same way. Expic: None Release note: None
miretskiy
pushed a commit
to miretskiy/cockroach
that referenced
this pull request
Sep 8, 2023
Extend metric registry to allow metric slices to be used. Prior to this change metric registry only alowed arrays. Slices are treated identical to the arrays -- the values are expanded, and each metric added to the registry. The array itself was not added. From some historical archeological perspective, cockroachdb#46747 added support for embedded metric Arrays. My best read on this PR is that it narrowly addressed the existing "TODOs", and avoided doing anything extra (which is good!). But it does not appear that there is a fundamental reason why slices should not be supported the same way. Expic: None Release note: None
wenyihu6
pushed a commit
to wenyihu6/cockroach
that referenced
this pull request
Jul 21, 2024
Extend metric registry to allow metric slices to be used. Prior to this change metric registry only alowed arrays. Slices are treated identical to the arrays -- the values are expanded, and each metric added to the registry. The array itself was not added. From some historical archeological perspective, cockroachdb#46747 added support for embedded metric Arrays. My best read on this PR is that it narrowly addressed the existing "TODOs", and avoided doing anything extra (which is good!). But it does not appear that there is a fundamental reason why slices should not be supported the same way. Expic: None Release note: None # Conflicts: # pkg/util/metric/registry.go # pkg/util/metric/registry_test.go
wenyihu6
pushed a commit
to wenyihu6/cockroach
that referenced
this pull request
Jul 23, 2024
Extend metric registry to allow metric slices to be used. Prior to this change metric registry only alowed arrays. Slices are treated identical to the arrays -- the values are expanded, and each metric added to the registry. The array itself was not added. From some historical archeological perspective, cockroachdb#46747 added support for embedded metric Arrays. My best read on this PR is that it narrowly addressed the existing "TODOs", and avoided doing anything extra (which is good!). But it does not appear that there is a fundamental reason why slices should not be supported the same way. Expic: None Release note: None # Conflicts: # pkg/util/metric/registry.go # pkg/util/metric/registry_test.go
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
This commit adds a new counter metric for each of the 44 request methods. It then updates DistSender to track the use of these methods in their associated counters.
The goal of this is to provide observability into the types of RPCs that are being sent in a cluster. We've seen a few cases in the past month where a cluster was sending more local and remote RPCs than expected, and being able to easily see what operations these RPCs were performing would have been a significant help to debugging.
Release note (admin ui change): A collection of new metrics were added that track the method of requests that are sent as RPCs between nodes in a cluster. These metrics are named
distsender.rpc.<method>.sent, with examples of<method>being "scan" and "conditionalput".