feat(token): add metrics and traces for cctp and lombard - #1409
Open
makramkd wants to merge 3 commits into
Open
Conversation
- Add per-message attestation fetch counter + duration histogram for cctp/lombard verifiers (outcome: success/not_ready/not_found/error) - Add outbound attestation HTTP client metrics: active requests, requests by method/outcome/status, duration, rate-limited counter, and API cooldown gauge - Open per-message attestation fetch spans under the task-verifier attempt span so they extend the source reader's base message trace - Add span events (fetch started/failed/not_found/not_ready/succeeded) and provider/status/result attributes - Add a child span around each outbound attestation HTTP request in the shared token HTTP client
makramkd
marked this pull request as ready for review
September 4, 2026 10:06
|
Code coverage report:
|
bukata-sa
reviewed
Sep 4, 2026
Comment on lines
+505
to
+509
| vm.tokenHTTPRequestDurationSeconds, err = beholder.GetMeter().Float64Histogram( | ||
| "verifier_token_http_request_duration_seconds", | ||
| metric.WithDescription("Duration of outbound attestation HTTP requests"), | ||
| metric.WithUnit("seconds"), | ||
| ) |
Collaborator
There was a problem hiding this comment.
Can you check if in VM it is verifier_token_http_request_duration_seconds and not verifier_token_http_request_duration_seconds_seconds or something else
bukata-sa
reviewed
Sep 4, 2026
| sdkmetric.NewView( | ||
| sdkmetric.Instrument{Name: "verifier_token_attestation_fetch_duration_seconds"}, | ||
| sdkmetric.Stream{Aggregation: sdkmetric.AggregationExplicitBucketHistogram{ | ||
| Boundaries: []float64{0.001, 0.005, 0.01, 0.025, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10, 25, 50}, |
Collaborator
There was a problem hiding this comment.
do we have a default timeout for attestation fetch? I think windows bigger than 5s won't be utilized
0xsuryansh
approved these changes
Sep 4, 2026
bukata-sa
reviewed
Sep 4, 2026
Comment on lines
+114
to
+116
| if attemptSC := oteltrace.SpanContextFromContext(task.TraceContext); attemptSC.IsValid() { | ||
| parentCtx = oteltrace.ContextWithSpanContext(ctx, attemptSC) | ||
| } |
Collaborator
There was a problem hiding this comment.
I have a helper method
TraceContextForMessage(tCtx, messageID)
In which cases attemptSC can be invalid? don't have this check anywhere
bukata-sa
reviewed
Sep 4, 2026
| // task.TraceContext is derived from context.WithoutCancel, so inject the attempt | ||
| // span context into ctx to keep its deadline/cancellation for the actual fetch. | ||
| parentCtx := ctx | ||
| if task.TraceContext != nil { |
Collaborator
There was a problem hiding this comment.
we should always have the TraceContext. Reading from db preserving the parent-child relations, it just loses the span itself (and ability to update existing span)
0xsuryansh
approved these changes
Sep 4, 2026
bukata-sa
approved these changes
Sep 4, 2026
| // must not be silently dropped because the (often short) API timeout fired | ||
| // while we were measuring the request. | ||
| mCtx := context.WithoutCancel(ctx) | ||
| _, span := otel.GetTracerProvider().Tracer("ccv.token.http").Start(ctx, "token_http_request", |
Collaborator
There was a problem hiding this comment.
we are using only beholder.GetTracer() (available globally)
Is there some idea to not use beholder one?
| // must not be silently dropped because the (often short) API timeout fired | ||
| // while we were measuring the request. | ||
| mCtx := context.WithoutCancel(ctx) | ||
| _, span := otel.GetTracerProvider().Tracer("ccv.token.http").Start(ctx, "token_http_request", |
Collaborator
There was a problem hiding this comment.
shouldn't be mCtx passed to the span too?
| // mCtx is deliberately detached from the request's deadline: metric records | ||
| // must not be silently dropped because the (often short) API timeout fired | ||
| // while we were measuring the request. | ||
| mCtx := context.WithoutCancel(ctx) |
Collaborator
There was a problem hiding this comment.
I don't really think metrics make some requests using provided context. Is it a best-practice to not pass cancellable context to the methods if you don't want them to be interrupted?
| // (which keeps its deadline/cancellation) rather than using task.TraceContext | ||
| // directly, since that is derived from context.WithoutCancel. | ||
| parentCtx := ctx | ||
| if task.TraceContext != nil { |
Collaborator
There was a problem hiding this comment.
same thoughts as for CCTP
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
3 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.
Description
Testing
I tested this out on devenv in the following situations:
This is what the span looks like for the happy path:
And this is what it looks like with some retries:
Checklist
changelogdirectory)