feat: populate metric labels from span context using metrics-tracing-context - #2634
feat: populate metric labels from span context using metrics-tracing-context#2634carneiro-cw wants to merge 11 commits into
Conversation
…ing point-in-time to some metrics
![cloudwalk-review-agent[bot]](https://avatars.githubusercontent.com/u/6581007?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Good direction overall: wiring tracing span fields into metrics labels is useful, and moving metrics init into the Tokio runtime context is necessary for the spawned exporter task. I found one blocking correctness issue in the span field population that can prevent the intended label from ever being attached to eth_call metrics.
Blocking
eth_callcreates apoint_in_timespan field, but the value is recorded inrpc_callbefore that method span exists, so the field is never set on theeth_callspan. As implemented,Span::with(...)inrpc_callrecords on the current span at that point in execution (middleware/no method span yet), so metrics labels forpoint_in_timeoneth_callcan be missing/empty despite being declared.
Please ensure the point_in_time recording happens after entering the eth_call method span (or pass/record via explicit span handle) so the metrics-tracing-context layer can observe it consistently.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad93bf7b66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let (recorder, exporter) = builder.build()?; | ||
| tokio::spawn(exporter); | ||
|
|
||
| let recorder = MetricsTracingContextLayer::only_allow(["rpc_client", "rpc_method", "point_in_time"]).layer(recorder); |
There was a problem hiding this comment.
When metrics are enabled and an RPC arrives, this recorder adds rpc_client and rpc_method to every metric recorded in the request span, including set_rpc_requests_active in rpc_middleware.rs. That gauge is set to the server-wide active connection count, so the change creates a separate series for each client/method whose last global value remains stale; consumers can no longer obtain the current active total from this metric and sums will overcount. Exclude this gauge from context enrichment or change it to maintain genuinely per-label counts.
Useful? React with 👍 / 👎.
|
|
||
| // execute | ||
| let point_in_time = ctx.server.storage.translate_to_point_in_time(filter)?; | ||
| Span::with(|s| s.rec_str("point_in_time", &point_in_time)); |
There was a problem hiding this comment.
When rpc_call is reached through the stratus_call handler, this recording is a no-op because that handler's rpc::stratus_call span declares only tx_from, tx_to, and filter; tracing cannot dynamically add an undeclared field. Consequently metrics emitted while executing stratus_call lack the new point_in_time label even though the same helper supplies it for eth_call. Add the empty field to the stratus_call span as well.
Useful? React with 👍 / 👎.
|
Benchmark: Git Info:
Leader Stats: Follower Stats: Plots: |
|
Benchmark: Git Info:
Leader Stats: Follower Stats: Plots: |
2 participants
No description provided.