Conversation
|
👋 AndresJulia, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
| monitoring.Metrics().IncrementActiveRequestsCounter(c.Request.Context()) | ||
|
|
||
| // Increment HTTP request counter | ||
| monitoring.Metrics().IncrementHTTPRequestCounter(c.Request.Context()) |
There was a problem hiding this comment.
because we have counter in RecordHTTPRequestDuration
There was a problem hiding this comment.
do we want to record HttpRequestDuration on all endpoints? I think the duration should be more precisely set to particular endpoints since it has increased cardinality, while we keep httprequestcounter as a more generic one, for all endpoints
There was a problem hiding this comment.
Got it, yeah makes sense
@AndresJulia can you do this
- keep counter for all endpoints in middleware + record http status
- move http request duration to handler and instead of URI record path from predefined set of values:
/verifierresults,/verifierresults/:messageID,/messages
| sdkmetric.Instrument{Name: "indexer_storage_query_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}, | ||
| Boundaries: []float64{0.001, 0.005, 0.01, 0.05, 0.1, 0.5, 1, 5, 10}, |
There was a problem hiding this comment.
i know it's kinda default, but is there any value in distinction between 1ms, 5ms, or 10ms calls?
| } | ||
|
|
||
| i.monitoring.Metrics().RecordStorageQueryDuration(ctx, time.Since(startQueryMetric)) | ||
| i.monitoring.Metrics().RecordStorageQueryDuration(ctx, time.Since(startQueryMetric), opName) |
There was a problem hiding this comment.
to have full visibility, we should probably also include the query status (error=(true|false))
There was a problem hiding this comment.
there is another metric for errors
There was a problem hiding this comment.
I agree with Marcin here, for latency we also might need a status here and in the storage query duration metric
I'm wondering if we can drop error metric in this case
|
@AndresJulia, please make sure you merge main to your PR. I've moved some logic from indexer middleware to the shared packages, and now TokenVerifier relies on the same shared code (unified way of tracking API latencies) Context: #676 |
AndresJulia
force-pushed
the
CCIP-9200_indexer_metric_gaps
branch
from
04bbc54 to
a7969e1
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
Code coverage report:
WARNING: go tool cover failed for coverage_target.out |
4 participants
addressing items listed in https://smartcontract-it.atlassian.net/browse/CCIP-9290