Skip to content
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

Merged
merged 6 commits into from
Aug 22, 2024

Conversation

feuGeneA
Copy link
Contributor

as discussed on standup

Why this should be merged

How this works

How this was tested

How is this documented

@@ -31,6 +31,7 @@ var Opts = struct {
InvalidSignatureResponses prometheus.CounterOpts
SignatureCacheHits prometheus.CounterOpts
SignatureCacheMisses prometheus.CounterOpts
CachedSignatureWeightPercentage prometheus.GaugeOpts
Copy link
Contributor

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.

Base automatically changed from signature-aggregation-api-cache to main August 16, 2024 18:45
@feuGeneA feuGeneA changed the title add CachedSignatureWeightPercentage metric signature-aggregator: add ConnectedStakeWeightPercentage metric Aug 21, 2024
@@ -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)
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation

Copy link
Contributor

@bernard-avalabs bernard-avalabs left a 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 <[email protected]>
Signed-off-by: F. Eugene Aumson <[email protected]>
@feuGeneA feuGeneA force-pushed the signature-aggregation-api-cache-sig-weight-metric branch from 9ec2b44 to f964b69 Compare August 21, 2024 19:17
@feuGeneA feuGeneA merged commit 3272c9d into main Aug 22, 2024
7 checks passed
@feuGeneA feuGeneA deleted the signature-aggregation-api-cache-sig-weight-metric branch August 22, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants