-
Notifications
You must be signed in to change notification settings - Fork 150
Track pubsub topic validator receives - F-ssv-1044 #2843
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
base: stage
Are you sure you want to change the base?
Changes from all commits
57d7213
716103e
b88c35e
a925ed5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,8 @@ | ||||||||||||
| package topics | ||||||||||||
|
|
||||||||||||
| import ( | ||||||||||||
| "context" | ||||||||||||
|
|
||||||||||||
| "go.opentelemetry.io/otel" | ||||||||||||
| "go.opentelemetry.io/otel/attribute" | ||||||||||||
| "go.opentelemetry.io/otel/metric" | ||||||||||||
|
|
@@ -12,6 +14,9 @@ import ( | |||||||||||
| const ( | ||||||||||||
| observabilityName = "github.com/ssvlabs/ssv/network/topics" | ||||||||||||
| observabilityNamespace = "ssv.p2p.messages" | ||||||||||||
|
|
||||||||||||
| pubsubObservabilityNamespace = "ssv.p2p.pubsub.messages" | ||||||||||||
| pubsubTopicAttributeKey = "ssv.p2p.pubsub.topic" | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Attribute-key fragmentation for "topic name" (informational — not blocking) This new key labels exactly the same logical thing (a libp2p topic name) as two existing keys in the codebase:
With this PR there are now three keys for the same value across the P2P metrics surface. The most natural correlation an operator wants — "received → inbound → peers per topic" — cannot be done with a single I understand the rationale (the new |
||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| var ( | ||||||||||||
|
|
@@ -29,13 +34,23 @@ var ( | |||||||||||
| metric.WithUnit("{message}"), | ||||||||||||
| metric.WithDescription("total number of outbound(broadcasted) messages"))) | ||||||||||||
|
|
||||||||||||
| pubsubMessagesReceivedCounter = metrics.New( | ||||||||||||
| meter.Int64Counter( | ||||||||||||
| observability.InstrumentName(pubsubObservabilityNamespace, "received"), | ||||||||||||
| metric.WithUnit("{message}"), | ||||||||||||
| metric.WithDescription("total number of messages delivered to the pubsub topic validator, before SSV validation runs (compare with ssv_p2p_messages_in_total for the post-validation rate)"))) | ||||||||||||
|
|
||||||||||||
| msgIDHandlerBufferFallbackCounter = metrics.New( | ||||||||||||
| meter.Int64Counter( | ||||||||||||
| observability.InstrumentName(observabilityNamespace, "msg_id_buffer_fallback"), | ||||||||||||
| metric.WithUnit("{event}"), | ||||||||||||
| metric.WithDescription("total number of msg_id add operations processed synchronously because the async buffer was full"))) | ||||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| func pubsubTopicAttribute(value string) attribute.KeyValue { | ||||||||||||
| return attribute.String(pubsubTopicAttributeKey, value) | ||||||||||||
| } | ||||||||||||
|
julienharbulot marked this conversation as resolved.
|
||||||||||||
|
|
||||||||||||
| func messageTopicAttribute(value string) attribute.KeyValue { | ||||||||||||
| return attribute.String("ssv.p2p.message.topic", value) | ||||||||||||
| } | ||||||||||||
|
|
@@ -46,3 +61,10 @@ func messageTypeAttribute(value uint64) attribute.KeyValue { | |||||||||||
| Value: observability.Uint64AttributeValue(value), | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // recordPubsubMessageReceived is called from the topic validator wrapper before the inner SSV | ||||||||||||
| // validator runs, so the counter increments for every message libp2p hands to the validator | ||||||||||||
| // regardless of validation outcome (accept/ignore/reject/timeout). | ||||||||||||
| func recordPubsubMessageReceived(ctx context.Context, topic string) { | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor: worth a 1-line comment to flag pre-validation timing A future reader touching the wrapper in
Suggested change
|
||||||||||||
| pubsubMessagesReceivedCounter.Add(ctx, 1, metric.WithAttributes(pubsubTopicAttribute(topic))) | ||||||||||||
| } | ||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| package topics | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| "go.opentelemetry.io/otel" | ||
| "go.opentelemetry.io/otel/sdk/metric" | ||
| "go.opentelemetry.io/otel/sdk/metric/metricdata" | ||
| ) | ||
|
|
||
| func TestRecordPubsubMessageReceived(t *testing.T) { | ||
| reader := metric.NewManualReader() | ||
| provider := metric.NewMeterProvider(metric.WithReader(reader)) | ||
| previousProvider := otel.GetMeterProvider() | ||
| otel.SetMeterProvider(provider) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we consider moving the provider swap into a Seems that setting the global provider mid-test leaves the package-level counters re-pointed for whatever runs after, and restoring the previous (delegating) provider in cleanup may not actually rebind them. Setting a |
||
| t.Cleanup(func() { | ||
| otel.SetMeterProvider(previousProvider) | ||
| require.NoError(t, provider.Shutdown(t.Context())) | ||
| }) | ||
|
|
||
| const topic = "ssv.v2.42" | ||
| recordPubsubMessageReceived(t.Context(), topic) | ||
| recordPubsubMessageReceived(t.Context(), topic) | ||
|
|
||
| var rm metricdata.ResourceMetrics | ||
| require.NoError(t, reader.Collect(t.Context(), &rm)) | ||
|
|
||
| for _, scopeMetrics := range rm.ScopeMetrics { | ||
| for _, m := range scopeMetrics.Metrics { | ||
| if m.Name != "ssv.p2p.pubsub.messages.received" { | ||
| continue | ||
| } | ||
|
|
||
| sum, ok := m.Data.(metricdata.Sum[int64]) | ||
| require.True(t, ok) | ||
| require.Len(t, sum.DataPoints, 1) | ||
| require.EqualValues(t, 2, sum.DataPoints[0].Value) | ||
|
|
||
| topicAttr, ok := sum.DataPoints[0].Attributes.Value(pubsubTopicAttributeKey) | ||
| require.True(t, ok) | ||
| require.Equal(t, topic, topicAttr.AsString()) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| t.Fatal("pubsub received metric was not collected") | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
Might be worth a test that goes through the wrapper itself — the new test calls
recordPubsubMessageReceiveddirectly, so the wiring here stays unverified.Registering the validator and pushing one message through it, then asserting the counter, would close that gap.