feat: populate metric labels from span context using metrics-tracing-context

[gventino-cw]

PR Type

#2579

Enhancement, Tests

---

Description

• instrument RPC spans with client and point_in_time fields

• add Prometheus exporter and tracing context recorder

• implement StratusMetricsLabelFilter allowlist with tests

---

Diagram Walkthrough

flowchart LR
  Mw["RpcMiddleware"] -- "record client & rpc_id" --> Sp["Span: RPC methods"]
  Sp -- "include point_in_time" --> Tg["TracingContextLayer"]
  Tg -- "attach metrics recorder" --> Ex["Prometheus exporter"]
  Ex -- "serve metrics" --> EP["Metrics endpoint"]

Loading

File Walkthrough

	Relevant files
Enhancement	rpc_middleware.rs Instrument middleware with client field                                    src/eth/rpc/middleware/rpc_middleware.rs add client field to info_span! macro record client via s.rec_str in middleware +2/-0      server.rs Add point_in_time to RPC method spans                                        src/eth/rpc/server.rs include point_in_time in eth_estimateGas and eth_call spans record point_in_time with Span::with calls +17/-2    metrics_config.rs Initialize metrics exporter and label filter                          src/infra/metrics/metrics_config.rs refactor exporter init using install_metrics_tracing_recorder add StratusMetricsLabelFilter allowlist implementation include tests for label filter behavior +110/-5  tracing_config.rs Enable metrics tracing context layer                                          src/infra/tracing/tracing_config.rs enable MetricsTracingFieldsLayer when metrics feature active include metrics layer in tracing subscriber registry +21/-5
Dependencies	Cargo.toml Add metrics-tracing-context and util dependencies                Cargo.toml add metrics-tracing-context and metrics-util optional deps extend metrics feature to include new dependencies +3/-1

---

[cloudwalk-review-agent]

Summary

Nice improvement to metrics context propagation. The PR correctly wires metrics-tracing-context into tracing subscriber setup, adds a conservative allowlist (client, rpc_method, point_in_time) to avoid high-cardinality metric labels, and includes focused tests for the filter behavior. I also verified the added RPC span fields (client, point_in_time) are recorded where needed to support those labels.

I did not find concrete correctness, security, or deploy-safety issues in the provided diff.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Tokio spawn naming The Prometheus exporter task is started with handle.spawn(exporter) without a name. For better observability and monitoring, use spawn_named (or spawn_blocking_named) to attach a clear task name and ensure it appears correctly in tracing. handle.spawn(exporter);

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Remove duplicate span label The code records the same &client value twice under both "client" and "rpc_client" labels. Remove the redundant label or rename one to use the correct variable if they are meant to differ. Consolidating these will avoid duplicated metadata in your spans. src/eth/rpc/middleware/rpc_middleware.rs [222-227] Span::with(|s| { - s.rec_str("client", &client); + s.rec_str("rpc_client", &client); s.rec_str("rpc_id", &request.id); - s.rec_str("rpc_client", &client); s.rec_str("rpc_method", &method); if let Some(ref tx) = tx { ... } }); Suggestion importance[1-10]: 5 __ Why: The span records both client and rpc_client with the same value, causing duplicated metadata in tracing context.	Low

[cloudwalk-review-agent]

Summary

LGTM. The PR cleanly wires tracing-context-to-metrics propagation with an explicit allowlist, and the added tests cover both positive and negative label-filter behavior (including protection against high-cardinality RPC labels). I also verified the new client and point_in_time span fields are declared and recorded where needed for the intended local-call metrics.

No concrete correctness, security, or deploy-safety issues were identified in the provided diff.

[cloudwalk-review-agent]

Summary

Good direction overall: the PR wires tracing context into metrics with a tight allowlist (client, rpc_method, point_in_time) and adds focused tests to prevent high-cardinality leakage. The new RPC span fields also look consistent with the intended metric enrichment.

I found one blocking correctness/deploy-safety issue in metrics initialization.

Blocking

1. Metrics exporter startup failures are swallowed, but MetricsConfig::init() still returns Ok(()).
   In init_metrics_exporter, errors from install_metrics_tracing_recorder(builder) are only logged; meanwhile MetricsConfig::init() cannot observe the failure and proceeds to register metric descriptions as if metrics were active. This can silently boot without metrics in environments expecting them, making observability regressions hard to detect. Please propagate the error up (or explicitly fail startup under metrics feature) instead of only logging.

[cloudwalk-review-agent]

Summary

Good evolution overall: the PR wires tracing context into metrics with an explicit label allowlist and adds focused tests to avoid high-cardinality leakage.

I found one blocking deploy-safety/correctness issue still present.

Blocking

1. Metrics initialization errors are still swallowed, so startup can succeed with metrics silently disabled.
   init_metrics_exporter logs errors from install_metrics_tracing_recorder(builder) but returns (), and MetricsConfig::init() always returns Ok(()). With this flow, failures in exporter/recorder setup (e.g., bind error, global recorder already set) won’t fail startup even when metrics is enabled, causing silent observability regressions.

Please propagate the error back through init_metrics_exporter/MetricsConfig::init() (or explicitly document/guard a non-fatal policy) so runtime behavior is deterministic.

[cloudwalk-review-agent]

Summary

Good progress on wiring tracing context into metrics and moving metrics initialization inside the Tokio runtime. The client and point_in_time span fields are now consistently recorded and the only_allow filter reduces cardinality risk.

I still found one blocking deploy-safety issue.

Blocking

1. Metrics exporter/recorder initialization failures are still swallowed, so startup can succeed with metrics silently disabled.
   In init_metrics_exporter, errors from install_metrics_tracing_recorder(builder) are logged but not propagated, and MetricsConfig::init() still returns success. This keeps the previous failure mode where bind/recorder setup errors (including global recorder conflicts) can pass unnoticed in production when metrics is enabled.

Please return anyhow::Result<()> from init_metrics_exporter and propagate it through MetricsConfig::init() so startup behavior is deterministic (or explicitly gate/document non-fatal behavior).

[carneiro-cw]

PointInTime can be MinedPast(number), which has unbounded cardinality. When used for labels we should just record MinedPast, Latest or Pending.

[cloudwalk-review-agent]

Summary

Nice evolution overall: tracing fields (rpc_client, point_in_time) are now wired through to metrics context, and metrics init was moved under the Tokio runtime so spawning the exporter is valid. I found one blocking deploy-safety issue still present.

Blocking

1. Metrics exporter initialization failures are still swallowed, allowing startup to succeed with metrics silently broken.
   install_metrics_tracing_recorder(builder) returns Result, but init_metrics_exporter logs and suppresses errors, and MetricsConfig::init() still returns Ok(()) regardless. That keeps non-deterministic boot behavior for bind failures or set_global_recorder conflicts, which is risky in production observability and was the prior blocker.

Please propagate the error path (e.g., make init_metrics_exporter return anyhow::Result<()> and ? it from MetricsConfig::init()) so startup either fully enables metrics or fails explicitly.

[gventino-cw]

PointInTime can be MinedPast(number), which has unbounded cardinality. When used for labels we should just record MinedPast, Latest or Pending.

We don't need to do it, the PointInTime enum and MinedPointInTime enum impl the strum::Display trait:

/// EVM storage point-in-time indicator.
#[derive(Debug, strum::Display, Clone, Copy, Default, strum::EnumIs, serde::Serialize, PartialEq, Eq)]
#[cfg_attr(test, derive(fake::Dummy))]
pub enum PointInTime {
    /// State of `Account` or `Slot` at the pending block being mined.
    ///
    /// If the pending state did not change, then it is the same as the `Mined` state.
    #[strum(to_string = "pending")]
    Pending,

    /// State of `Account` or `Slot` at the last mined block.
    #[default]
    #[strum(to_string = "mined")]
    Latest,

    /// State of `Account` or `Slot` at some specific mined block in the past.
    #[strum(to_string = "mined-past")]
    Past(BlockNumber),
}

#[derive(Debug, strum::Display)]
pub enum MinedPointInTime<'a> {
    #[strum(to_string = "latest")]
    Latest(Seal, Option<RwLockReadGuard<'a, ()>>),
    #[strum(to_string = "past")]
    Past(Seal, BlockNumber),
}

stratus_storage_read_block{service="stratus-stratus",version="feat/metrics::8d119279",rpc_client="other::test",rpc_method="eth_call",point_in_tim'e="mined-past",group="storage_read",node_mode="leader",storage="permanent",success="true",quantile="0"} 0.0001425

[...]

stratus_storage_read_account{service="stratus-stratus",version="feat/metrics::8d119279",rpc_client="other::test:",rpc_method="eth_call",point_in_time="past",group="storage_read",node_mode="leader",storage="permanent",hit="false",quantile="0"} 0.000000334

[cloudwalk-review-agent]

Summary

Looks good overall. The previously blocking deploy-safety issue is addressed: metrics exporter/recorder setup errors are now propagated via anyhow::Result from init_metrics_exporter into MetricsConfig::init, making startup behavior deterministic instead of silently disabling metrics.

I also verified the metrics/tracing context wiring is coherent under the metrics feature:

• MetricsLayer is attached to the tracing subscriber (tracing_config.rs) to capture span fields.
• Prometheus recorder is wrapped with TracingContextLayer::only_allow(["rpc_client", "rpc_method", "point_in_time"]) to bound label cardinality.
• point_in_time is explicitly recorded on eth_call/eth_estimateGas spans in server.rs.

No concrete blocking issues found in the provided diff context.

[cloudwalk-review-agent]

Summary

Looks good to me.

This revision addresses the previous deploy-safety concern by propagating metrics exporter initialization errors (anyhow::Result + ?) instead of swallowing them, and it initializes metrics within the Tokio runtime before spawning the exporter task. The tracing-to-metrics wiring is coherent under the metrics feature (MetricsLayer on the tracing subscriber + TracingContextLayer::only_allow(["rpc_client", "rpc_method", "point_in_time"]) on the recorder), which also bounds label cardinality.

I did not find any new concrete correctness/security/regression issues in the provided diff.

[cloudwalk-review-agent]

Summary

LGTM. This revision correctly wires tracing span fields into metrics context under the metrics feature and keeps cardinality bounded via only_allow(["rpc_client", "rpc_method", "point_in_time"]). I also verified the earlier deploy-safety concern is still addressed: metrics exporter initialization now propagates errors (anyhow::Result + ?), and metrics init runs inside the Tokio runtime before tokio::spawn(exporter).

I did not find concrete correctness, security, concurrency, or deploy-safety regressions in the provided diff context.