Skip to content

Add lean_attestation_committee_count Prometheus metric#223

Merged
pablodeymo merged 4 commits intomainfrom
feat/add-attestation-committee-count-metric
Mar 13, 2026
Merged

Add lean_attestation_committee_count Prometheus metric#223
pablodeymo merged 4 commits intomainfrom
feat/add-attestation-committee-count-metric

Conversation

@pablodeymo
Copy link
Collaborator

Motivation

The leanMetrics spec defines a `lean_attestation_committee_count` gauge metric (Network Metrics category) that reports the number of attestation committees. It is collected on node start with no labels. No client has implemented it yet.

The Grafana dashboard in leanMetrics already references it:
```
max(lean_attestation_committee_count{job=~"$job"})
```

Description

  • Adds `set_attestation_committee_count(count: u64)` to `blockchain/src/metrics.rs`, registering a `lean_attestation_committee_count` IntGauge
  • Calls it from `main.rs` on node start, alongside the existing `set_node_info` and `set_node_start_time` calls
  • The value comes from the existing `--attestation-committee-count` CLI option (default: 1)

How to Test

  1. `cargo test --workspace --release` — all tests pass
  2. `cargo clippy --workspace -- -D warnings` — clean
  3. `cargo fmt --all -- --check` — clean
  4. Run the node and verify the metric appears at `/metrics`:
    ```
    curl -s http://localhost:9090/metrics | grep lean_attestation_committee_count
    ```

Expose the attestation committee count as a Prometheus gauge on node
start, matching the leanMetrics spec. The value comes from the existing
--attestation-committee-count CLI option.
@github-actions
Copy link

🤖 Kimi Code Review

Review Summary

The PR adds a new metric to track the attestation committee count. The implementation is straightforward and follows the existing patterns in the codebase.

Issues Found

  1. Potential overflow in type conversion (crates/blockchain/src/metrics.rs:371)

    • The code casts u64 to i64 without checking for overflow. While unlikely in practice (committee counts are typically small), this could panic on values > i64::MAX.
    • Fix: Add a saturating cast or explicit bounds check:
      LEAN_ATTESTATION_COMMITTEE_COUNT.set(count.try_into().unwrap_or(i64::MAX));
  2. Metric naming inconsistency (crates/blockchain/src/metrics.rs:365)

    • The metric name uses "lean_" prefix while the constant uses "LEAN_" prefix, but this is inconsistent with some other metrics in the file (e.g., set_is_aggregator uses LEAN_IS_AGGREGATOR but metric name is lean_is_aggregator).
    • Note: This appears to be the established pattern in this codebase, so no change needed unless you want to standardize.
  3. Missing validation of input parameter (bin/ethlambda/src/main.rs:85-87)

    • The attestation committee count is set directly from CLI options without validation. While the metric itself won't fail, invalid values (like 0) could be misleading.
    • Fix: Consider adding validation in the CLI argument parsing or here:
      if options.attestation_committee_count > 0 {
          ethlambda_blockchain::metrics::set_attestation_committee_count(
              options.attestation_committee_count,
          );
      }

Positive Aspects

  • Follows the established pattern for metrics in the codebase
  • Uses LazyLock for efficient initialization
  • Properly integrates with the existing metrics system
  • Clean, minimal implementation

Automated review by Kimi (Moonshot AI) · custom prompt

@github-actions
Copy link

🤖 Codex Code Review

  1. Low – unchecked u64 -> i64 cast can produce negative metric values
    At crates/blockchain/src/metrics.rs:371, LEAN_ATTESTATION_COMMITTEE_COUNT.set(count as i64); will wrap for values > i64::MAX, yielding a negative gauge.
    This is not memory-unsafe, but it is a correctness/observability bug and can mislead ops/alerts.

Suggested fix:

  • Bound CLI input to <= i64::MAX at bin/ethlambda/src/main.rs:68, or
  • Use checked conversion in metrics setter (i64::try_from(count)), then clamp/log on overflow.

No other bugs or security/performance regressions are evident in this PR. It only adds metric plumbing and does not modify fork choice, attestation validation, finalization, state transition, XMSS, or SSZ paths.


Automated review by OpenAI Codex · custom prompt

@github-actions
Copy link

🤖 Claude Code Review

Here is the review:


PR Review: Add lean_attestation_committee_count Prometheus metric

Overall: Clean, small, well-motivated change. The implementation is correct and follows the established function-scoped LazyLock pattern. One minor nit on casting style.


crates/blockchain/src/metrics.rs

Minor: Inconsistent cast style

// New code (line 371)
LEAN_ATTESTATION_COMMITTEE_COUNT.set(count as i64);

The earlier u64-to-i64 count/slot conversions in this file use try_into().unwrap():

// Lines 9, 17, 25, 32, 43, 50
LEAN_HEAD_SLOT.set(slot.try_into().unwrap());
LEAN_VALIDATORS_COUNT.set(count.try_into().unwrap());

as i64 silently truncates if count > i64::MAX. While the default value of 1 makes overflow practically impossible, using try_into().unwrap() would be consistent with how other u64 counts are handled earlier in the file (lines 9–50) and makes the intent explicit.

Suggested fix:

LEAN_ATTESTATION_COMMITTEE_COUNT.set(count.try_into().unwrap());

bin/ethlambda/src/main.rs

The call site is correct — placed alongside the existing startup metrics calls, before the metrics server is spawned. No issues here.


Spec note

The PR description references curl -s http://localhost:9090/metrics but the default metrics port per the codebase is 5054. This is only in the description, not in code, so it is not a blocker — just worth a correction in the PR description to avoid confusing readers.


Summary: Approve with the try_into().unwrap() nit addressed for consistency.


Automated review by Claude (Anthropic) · custom prompt

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 13, 2026

Greptile Summary

This PR implements the lean_attestation_committee_count Prometheus gauge metric as specified by the leanMetrics spec, registering it as an IntGauge with no labels and setting it once at node start from the existing --attestation-committee-count CLI option.

  • crates/blockchain/src/metrics.rs: Adds set_attestation_committee_count(count: u64) following the established static LazyLock<IntGauge> pattern used throughout the file.
  • bin/ethlambda/src/main.rs: Calls the new function at startup alongside set_node_info and set_node_start_time, correctly reading the value from the already-existing options.attestation_committee_count field.
  • Minor style inconsistency: the new u64 parameter is cast with count as i64 (silent wrapping on overflow) while all other u64 parameters in the same file use .try_into().unwrap() (explicit panic on overflow). No practical impact given the expected range of committee counts.

Confidence Score: 5/5

  • This PR is safe to merge; the change is minimal, additive, and carries no runtime risk.
  • The change is a two-file addition that follows the well-established lazy-static gauge pattern already used extensively in the codebase. It touches no business logic, introduces no new dependencies, and the metric is set once at startup from a validated CLI option. The only finding is a minor as i64 vs .try_into().unwrap() style inconsistency that has no practical effect.
  • No files require special attention.

Important Files Changed

Filename Overview
crates/blockchain/src/metrics.rs Adds set_attestation_committee_count(count: u64) as an IntGauge following the established LazyLock pattern; minor style inconsistency using count as i64 instead of count.try_into().unwrap() used by all other u64-typed parameters in this file.
bin/ethlambda/src/main.rs Correctly calls set_attestation_committee_count at node startup alongside the existing set_node_info and set_node_start_time calls, passing the CLI-parsed attestation_committee_count value.

Sequence Diagram

sequenceDiagram
    participant CLI as CLI (CliOptions)
    participant Main as main.rs
    participant Metrics as metrics.rs (LazyLock)
    participant Prometheus as Prometheus Registry
    participant HTTP as /metrics endpoint

    CLI->>Main: parse() → attestation_committee_count: u64
    Main->>Metrics: set_attestation_committee_count(count)
    Metrics->>Prometheus: register_int_gauge!("lean_attestation_committee_count") [once]
    Metrics->>Prometheus: IntGauge.set(count as i64)
    Main->>HTTP: start_rpc_server(metrics_socket, store)
    HTTP-->>Prometheus: gather()
    Prometheus-->>HTTP: lean_attestation_committee_count <value>
Loading
Prompt To Fix All With AI
This is a comment left during a code review.
Path: crates/blockchain/src/metrics.rs
Line: 371

Comment:
**Inconsistent cast for `u64` parameter**

All other functions in this file that accept `u64` (slots, validator count) use `.try_into().unwrap()` for the `i64` conversion, which panics explicitly on overflow instead of silently wrapping. The new code uses `count as i64`, consistent only with the `usize`-based functions (`update_gossip_signatures`, etc.).

For a committee count this will never matter in practice, but it is inconsistent with the existing `u64` pattern in the file:

```suggestion
    LEAN_ATTESTATION_COMMITTEE_COUNT.set(count.try_into().unwrap());
```

How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: 0ae64dc

Copy link
Collaborator

@MegaRedHand MegaRedHand left a comment

Choose a reason for hiding this comment

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

We need to update docs/metrics.md

…style

- Document the new metric in the Network Metrics table
- Use try_into().unwrap() instead of `as i64` for consistency with
  other u64-to-i64 conversions in the file
A panic in a metrics setter is worse than a theoretical silent wrap
on a value that will never exceed i64::MAX in practice.
@pablodeymo pablodeymo force-pushed the feat/add-attestation-committee-count-metric branch from 151f2d0 to 8f065b7 Compare March 13, 2026 21:42
@pablodeymo
Copy link
Collaborator Author

Done — added lean_attestation_committee_count to the Network Metrics table in docs/metrics.md. Also switched the u64i64 cast to try_into().unwrap_or_default() for safety.

Copy link
Collaborator

@MegaRedHand MegaRedHand left a comment

Choose a reason for hiding this comment

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

LGTM

@pablodeymo pablodeymo merged commit c108c91 into main Mar 13, 2026
2 checks passed
@pablodeymo pablodeymo deleted the feat/add-attestation-committee-count-metric branch March 13, 2026 21:46
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.

2 participants