Skip to content

apollo_l1_gas_price: register each oracle metric set once - #14977

Open
asaf-sw wants to merge 1 commit into
main-v0.14.3from
asaf/l1-oracle-a4-metric-register-once
Open

apollo_l1_gas_price: register each oracle metric set once#14977
asaf-sw wants to merge 1 commit into
main-v0.14.3from
asaf/l1-oracle-a4-metric-register-once

Conversation

@asaf-sw

@asaf-sw asaf-sw commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

ExchangeRateOracleMetrics::register is called from ExchangeRateOracleClient::new, so every constructed client re-registers the static metric bundle for its pair (ETH_TO_STRK_ORACLE_METRICS, STRK_TO_USD_ORACLE_METRICS). This adds a per-bundle Once so each set registers exactly once per process.

Reachable path

A deployed node constructs one client per pair and never re-registers. The in-process multi-node path does: create_node_modules, the production composition function, is called once per simulated node inside a single test process by crates/apollo_integration_tests/src/flow_test_setup.rs:336. Each call rebuilds the per-pair clients against the same two static bundles, so node N's construction re-registers what node 1 already registered. The guard is insurance for exactly that in-process repeat construction.

Registering from ComponentStarter::start() instead (the way register_provider_metrics does) would not help: the flow-test harness starts each in-process node too.

On the strength of the claim

Worth being precise, because I checked it rather than assuming it. With the metrics stack actually in use, re-registration is currently harmless:

  • MetricCounter::register publishes the initial value via counter!(name).absolute(init), and CounterFn for AtomicU64 (metrics-0.24.2/src/atomics.rs:27) implements absolute as fetch_max. With init = 0 for all four oracle counters, a second register cannot lower an incremented counter.
  • MetricGauge::register only creates the handle and calls describe_gauge!; it sets no value.

I confirmed this empirically: with the Once removed, an incremented counter still survives a second register. So this PR is hardening, not a bug fix. What it buys is that register no longer depends on Counter::absolute being monotonic, which is a backend-defined detail (metrics leaves the semantics to the implementation), and it makes "registration happens once per process" an explicit property rather than an accident of the atomic storage backend.

If reviewers would rather not carry a guard for a latent-only issue, the alternative is to drop this PR. Flagging it rather than overselling the justification.

Changes

  • crates/apollo_l1_gas_price/src/metrics.rs: private registration_guard: &'static Once field on ExchangeRateOracleMetrics, register wrapped in call_once, one distinct Once static per bundle. Debug switches to finish_non_exhaustive since the guard is not printed.
  • crates/apollo_l1_gas_price/src/metrics_test.rs: constructs two clients for the same pair with an increment between, asserts the set's guard is uncompleted before the first construction and completed after it, and asserts the count survives the second construction. The guard is a test-owned Once so the state assertions do not depend on other tests in the process. Deleting the call_once wrapper makes the test fail. The test comment names the flow-test path it models.
  • crates/apollo_l1_gas_price/Cargo.toml: metrics, metrics-exporter-prometheus, and apollo_metrics with testing added as dev-dependencies for the new test.

Verification

  • ./scripts/rust_fmt.sh clean
  • RUSTFLAGS="-D warnings" cargo clippy -p apollo_l1_gas_price --all-targets --all-features clean
  • SEED=0 cargo nextest run -p apollo_l1_gas_price: 20 passed, 0 skipped
  • Negative control: with the call_once wrapper removed, repeated_client_construction_registers_metrics_once fails on assertion failed: TEST_REGISTRATION.is_completed()
  • scripts/taplo.sh and cargo machete clean

@reviewable-StarkWare

Copy link
Copy Markdown

This change is Reviewable

asaf-sw commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@asaf-sw
asaf-sw marked this pull request as ready for review August 17, 2026 22:12
@github-actions

Copy link
Copy Markdown

Artifacts upload workflows:

@matanl-starkware matanl-starkware left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matanl-starkware reviewed 4 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on asaf-sw).


crates/apollo_l1_gas_price/src/metrics.rs line 51 at r1 (raw file):

    pub last_success_timestamp: &'static MetricGauge,
    /// Guards this set's registration. Private so that every set is one of the constants below,
    /// each of which owns a distinct guard.

Suggestion:

    /// Guards this set's registration.

crates/apollo_l1_gas_price/src/metrics.rs line 58 at r1 (raw file):

    /// Runs the registration calls once per process per metric set. `MetricCounter::register`
    /// republishes the initial value through `Counter::absolute`, whose semantics the `metrics`
    /// facade leaves to the recorder; the guard removes the dependency on them.

crates/apollo_l1_gas_price/src/metrics.rs line 79 at r1 (raw file):

            .field("error_count", &self.error_count.get_name())
            .field("last_success_timestamp", &self.last_success_timestamp.get_name())
            .finish_non_exhaustive()

To be honest, I don't think that's necessary. A closing } is fine, silently omitting the guard.

Code quote:

.finish_non_exhaustive()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants