feat(telemetry): add node telemetry v1 MVP - #4641
Conversation
🟡 Heimdall Review Status
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /// The actor starts with an empty set, so the first fold counts the node's initial peers as | ||
| /// joins. That is the honest reading of "peers joined since this reporter started". | ||
| pub fn fold_peer_churn(&mut self, current: HashSet<String>) { | ||
| self.peers_joined += current.difference(&self.connected_peers).count() as u32; |
There was a problem hiding this comment.
nit: The as u32 casts on difference().count() can silently truncate if a HashSet has more than u32::MAX entries. In practice the peer set will never be that large, but u32::try_from(...).unwrap_or(u32::MAX) or a saturating cast would be more defensive. Same for peers_joined / peers_left which are u32 accumulators — consecutive folds with large peer sets could theoretically overflow via wrapping addition.
Low severity since peer counts are realistically small, but these are public methods.
| } | ||
|
|
||
| /// Returns whether this config will actually send anything. | ||
| pub const fn is_active(&self) -> bool { |
There was a problem hiding this comment.
is_active is const fn but calls Option::is_some(), which is a const fn in current Rust. However, this means is_active cannot evolve to include non-const checks (e.g. URL validation) without a breaking signature change. Minor, but worth noting since the method sits on a public configuration type.
|
|
||
| /// Returns the number of reports the background writer has dropped. | ||
| pub fn dropped(&self) -> u64 { | ||
| self.inner.errors.as_ref().map_or(0, |errors| errors.dropped_lines() as u64) |
There was a problem hiding this comment.
ErrorCounter::dropped_lines() returns usize, cast here as u64. On 32-bit platforms this is fine (widening), but on a hypothetical platform where usize > u64 it would truncate. Minor, but as u64 silently truncates in the general case — consider using .into() or u64::try_from().unwrap_or(u64::MAX) if the crate targets strict portability.
More importantly, ErrorCounter tracks errors across the writer's lifetime. report_new_drops uses swap to track what was previously observed, but dropped() returns the writer's total error count. If the error counter ever wraps (unlikely but theoretically possible for usize on 32-bit), the dropped > previous check would produce a false negative for one cycle. Not a practical concern for this use case.
| /// | ||
| /// A file that exists but does not hold a well-formed ID is replaced rather than treated as | ||
| /// fatal. A truncated write from a crash should not stop a node from starting. | ||
| pub fn load_or_create(path: &Path) -> Result<Self, TelemetryIdError> { |
There was a problem hiding this comment.
TOCTOU race on identity file: load_or_create first tries to read the file, and if it's absent, generates a new ID and writes it. If two processes (or a restart race) call this concurrently, both can read None, both generate different IDs, and the last writer wins — silently changing the node's identity.
This is unlikely in the single-process node case, but load_or_create is a pub method on a pub type. Consider using O_CREAT | O_EXCL (exclusive create) on the write path and retrying the read if the exclusive create fails, which makes the mint atomic on the filesystem.
Low severity since the PR description acknowledges identity is "reliable within a reporting window and not across months."
| #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] | ||
| pub struct NodeReportEvent { | ||
| /// When ingest accepted the report. | ||
| pub received_at: DateTime<Utc>, | ||
| /// The address attributed to the reporting node. | ||
| pub reported_ip: IpAddr, | ||
| /// Whether `reported_ip` came from the node or from the connection. | ||
| pub ip_source: IpSource, | ||
| /// The report as the node sent it. | ||
| #[serde(flatten)] | ||
| pub report: NodeReport, | ||
| } |
There was a problem hiding this comment.
NodeReportEvent uses #[serde(flatten)] on report: NodeReport, and NodeReport itself uses #[serde(flatten)] on client: ClientMeta. Nested flatten is known to have performance issues with serde (quadratic deserialization in some cases) and can produce confusing errors when field names collide. Currently all field names are distinct so correctness is fine, but adding a field to NodeReportEvent that collides with any NodeReport or ClientMeta field name will silently shadow it.
Consider documenting this constraint or adding a test that asserts no key collisions across the flattened levels (the existing test_the_payload_carries_exactly_the_specified_top_level_keys test partially covers NodeReport but not the NodeReportEvent wrapper).
| IngestApiError::from_json_rejection(&rejection) | ||
| })?; | ||
|
|
||
| if !report.is_current_schema() { | ||
| // Accepted anyway: an old or new node must keep reporting across a schema change, | ||
| // and every field that did parse is still worth having. | ||
| warn!( |
There was a problem hiding this comment.
The ingest_node_report handler accepts ConnectInfo<SocketAddr> which requires into_make_service_with_connect_info::<SocketAddr>() on the Axum server. This is correctly done in server.rs:140 and in the tests, but if someone wires this router without into_make_service_with_connect_info, the handler will panic at runtime on the first request rather than failing at compile time. This is an Axum footgun rather than a code issue, but worth noting since the IngestRoutes::router method is public and doesn't document this requirement.
Adds an end-to-end slice of node telemetry: a consensus node collects a full node report, POSTs it to the existing base-telemetry service, and the service records it as JSONL. New crates: - base-telemetry-types: wire schema only, shared by client and service so the two cannot drift. - base-telemetry-client: identity, hardware collection, latency sampling, and a ReportSink seam for transport. Wiring: - TelemetryActor in the consensus service, modeled on UpgradeSignalMetricsActor. It never returns Err, because spawn_and_wait! turns any actor error into a full node shutdown. - define_telemetry_args! in base-cli-utils, flattened into BaseCli. - `base telemetry preview` prints the exact payload a node would send. - /v1/ingest in base-telemetry-service behind the existing per-IP rate limiter, recording through a JsonlRecorder. Reports are read from typed in-process state rather than a scraped Prometheus registry: two mutually exclusive recorders exist in this tree and rendered metric names differ depending on which one wins. Ships dark. The actor is None unless --telemetry.endpoint is set, so merging this cannot cause any node to report anywhere. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
3e52d10 to
7c24992
Compare
| .join(".base") | ||
| .join(l2_chain_id.to_string()) | ||
| .join(TELEMETRY_ID_FILE_NAME) | ||
| } |
There was a problem hiding this comment.
nit: default_id_path falls back to PathBuf::from(".") when $HOME is unset, which puts the identity file at ./.base/<chain_id>/telemetry-id relative to the working directory. On a containerized node started by systemd, $HOME is often absent and the working directory is /, so the file lands at /.base/8453/telemetry-id. This works, but a container rebuild or a different working directory will silently mint a new identity. Consider using dirs_next::home_dir() (already a workspace dependency) which also checks /etc/passwd, or at least documenting that --telemetry.id-path should be set in container deployments.
|
|
||
| impl fmt::Debug for JsonlRecorder { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| f.debug_struct("JsonlRecorder") |
There was a problem hiding this comment.
The RecorderInner struct holds Option<WorkerGuard> as _guard. When the JsonlRecorder (and all its Arc clones) are dropped, this guard is dropped too, which flushes and joins the background writer thread. In server.rs, the recorder Arc lives for the lifetime of the server — fine.
However, there's a subtlety: ReportRecorder::record() calls writer.write_all(&line) on a cloned NonBlocking. If the WorkerGuard is dropped (e.g. during shutdown) while a concurrent record() call is in flight on another thread, write_all will fail because the internal channel is closed. This is handled by the if let Err(error) on line 170, so no panic. But report_new_drops() on line 172 may undercount because the ErrorCounter is also dropped with the guard.
Not a bug since shutdown ordering means no new requests arrive, but worth noting that the recorder should be the last thing dropped during graceful shutdown if accurate drop counts matter.
| self.peers_joined += current.difference(&self.connected_peers).count() as u32; | ||
| self.peers_left += self.connected_peers.difference(¤t).count() as u32; | ||
| self.connected_peers = current; | ||
| } |
There was a problem hiding this comment.
The report() method calls connected_peer_ids().await unconditionally at the top, then calls net_health().await which internally calls local_peer_info().await and discovered_peer_count().await. That's 3 sequential p2p RPC round trips per report cycle, each with a 5-second timeout.
In the worst case (p2p actor wedged), a single report cycle takes 15 seconds of wall-clock time before all three queries time out. With the default 15-minute report interval this is negligible, but if an operator configures a shorter interval (e.g. 10 seconds via --telemetry.report-interval), the report path could consume more than the full interval, causing report ticks to pile up and be skipped.
Consider either (a) issuing the 3 queries concurrently with tokio::join!, or (b) documenting a minimum report interval that exceeds 3 × P2P_QUERY_TIMEOUT.
Review SummaryThis PR adds a full telemetry stack: Block-production sensitivityThe FindingsAll findings are low severity. No critical issues found.
Overall assessmentWell-engineered telemetry MVP with strong safety properties. The "ships dark" approach, The existing inline comments from previous reviews cover the |
2 participants
Summary
This is the smallest end-to-end slice of node telemetry that proves the shape works: a consensus node collects a full node report, POSTs it to the existing
base-telemetryservice, and the service records it.It ships dark. The reporting actor is
Noneunless--telemetry.endpointis set, so merging this cannot cause any node to report anywhere. Turning sending on is a separate change.What changed
Two new crates.
base-telemetry-typesis the wire schema and nothing else, depended on by both the client and the service so the two cannot drift.base-telemetry-clientis collection and transport — identity, hardware, latency sampling, and aReportSinkseam — deliberately client-agnostic so the execution node and snapshot CLI can reuse it without a move.TelemetryActorin the consensus service is modeled onUpgradeSignalMetricsActor: sameNodeActorimpl, same interval-plus-cancellation select loop.define_telemetry_args!sits besidedefine_metrics_args!inbase-cli-utilsand flattens intoBaseCli.base telemetry previewprints the exact payload a node would send, so the payload is reviewable before anything is ever sent.On the service side,
/v1/ingestlands in the existingbase-telemetry-servicebehind the rate limiter already there, recording through aReportRecorderseam whose one implementation writes JSONL and emits a structured event per report.Design notes worth reviewing
The actor can never return
Err.spawn_and_wait!installs acancellation.drop_guard()per task, so any actor returningErrcancels the whole node. A telemetry failure taking down a Base node is the exact inversion of what this is for. Every recoverable error is logged and swallowed, and a test forces the sink to fail every call and asserts the actor stays up.This reads typed in-process state rather than scraping the Prometheus registry. Two mutually exclusive recorders exist in this tree —
BaseCli::runinstalls one, reth'sinstall_prometheus_recorder()installs another and.expect()s — and only one can winset_global_recorder. They also disagree on naming, since reth's wraps everything in aPrefixLayer::new("reth"). A scraper would have to know which recorder won in order to know what a metric is called. Every value in this payload already has a typed source: heads fromwatch::Receiver<EngineState>, peers fromP2pRpcRequest. Registry scraping earns its place when we want breadth across the ~40 crates usingdefine_metrics!; that is a follow-up, not a prerequisite.The wire format is snake_case, not the repo's camelCase HTTP convention. These become log facets, and the precedent is the transaction-event journal in
observability-events, which is snake_case NDJSON for the same reason. It is a log record, not a browser-facing API.schema_versionis an integer. The receiver rule is "reject an unknown major," which an exact string match cannot express.The telemetry ID is a UUID, and the migration is silent. Identities minted earlier were 32 undashed hex characters, which is exactly the simple UUID form, so an existing
telemetry-idfile parses unchanged and a node keeps its identity across the upgrade. There is a test pinning that.The payload is flat. Everything at the top level answers which node is this; everything in a nested block answers what is it reporting. That is the split consumers actually use — the top level is what you group and filter by, the blocks are what you aggregate. A test pins the exact top-level key set so the contract cannot drift silently.
Rate limits key on the edge IP, never on anything in the payload.
Testing
Unit coverage across the sampler, identity, hardware collector, reporter queue-drop behavior, and the actor's failure tolerance; ingest tests for 202, 400, 413, 429 and a JSONL round-trip; and an
etc/systemsend-to-end test.Draft because this has not been compiled since being rebased onto main. It was green beforehand — clippy clean,
cargo +nightly fmtclean, 395 lib tests plus 2 integration tests — and the rebase produced one trivial import conflict, but CI is the first real check on the merged result.cargo metadata --lockeddoes pass, so the manifests and lockfile are at least internally consistent.The system test needs a live devnet and has not been re-run since #4447 reshaped stack bring-up.
Not in this PR
Nothing enables sending. There is no public ingress and no remote kill switch, so this is deliberately not deployable to third-party nodes as-is. The execution-layer client is also out of scope; v1 is consensus-only.