-
Notifications
You must be signed in to change notification settings - Fork 23
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
signature-aggregator: add ConnectedStakeWeightPercentage metric #434
signature-aggregator: add ConnectedStakeWeightPercentage metric #434
Conversation
as discussed on standup
@@ -31,6 +31,7 @@ var Opts = struct { | |||
InvalidSignatureResponses prometheus.CounterOpts | |||
SignatureCacheHits prometheus.CounterOpts | |||
SignatureCacheMisses prometheus.CounterOpts | |||
CachedSignatureWeightPercentage prometheus.GaugeOpts |
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 feel like a Prometheus Gauge is not a great fit for this metric for consumption. It doesn't describe health/state of application as a whole but of current request. I do think that we want this data available but maybe a log would be better fit.
An idea for useful usage of gauges might be currently connected stake-weight percentage with subnets as labels.
…-api-cache-sig-weight-metric
@@ -193,7 +203,9 @@ func sampleMetrics(port uint16) map[string]uint64 { | |||
log.Debug("Found metric line", "line", line) | |||
parts := strings.Fields(line) | |||
|
|||
// Fetch the metric count from the last field of the line | |||
metricName = strings.Replace(parts[0], "U__signature_2d_aggregator_", "", 1) |
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.
JW how does this work with multiple subnets? Can we add a test case for that?
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.
note how I constructed the metric name using the subnet ID here: https://github.com/ava-labs/awm-relayer/pull/434/files#diff-973f1f8ae5b8f6acd388e8c2fdaa6e08f999b4211536e8e3258b5df24de79d04R135
in short, the metric name/key for the existing test case comes out as connected_stake_weight_percentage{subnetID="11111111111111111111111111111111LpoYY"}
.
If I understand correctly, we can't send a message from any subnets until the ACP-118 stuff is merged. (Right?) So at this time I can't add a test case for a different subnet, but I think the existing test does a good job of showing how it works.
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.
Thanks for the explanation
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.
Looks good to me other than Geoff's comment about ratio vs. percentage.
Co-authored-by: Geoff Stuart <geoff.vball@gmail.com> Signed-off-by: F. Eugene Aumson <feuGeneA@users.noreply.github.com>
9ec2b44
to
f964b69
Compare
addresses review comment #434 (comment)
as discussed on standup
Why this should be merged
How this works
How this was tested
How is this documented