feat: adds metrics interface

[S T (staheri14)]

Implements #109: the trait interface the node code will record metrics against. Interface only; no backend yet.

Included in the PR

• New observability module: the recording traits (SearchMetrics, LookupTableMetrics, NetworkMetrics) and their NodeMetrics composite, the label enums they use, and a unimock double per trait.
• Direction promoted to pub (now used in a public trait signature; this is why core/ is touched).

Follow-ups

• Backend (OTel + Prometheus): [Obs] Implement: OTel metrics & Prometheus exporter #110
• No-op: [Obs] Add: no-op/test-double metrics implementation #111
• Wiring: [Obs] Instrument: core search operations via metrics interface #112, [Obs] Instrument: node-level operations #113, [Obs] Instrument: network send/receive paths #114

Closes #109.

[TheP2P (thep2p)]

LGTM ❤️

[Copilot]

Pull request overview

Adds a new, backend-agnostic observability surface for Skip Graph nodes by introducing metrics-recording traits and bounded label enums, and exposes the observability module publicly so node code (and future backends) can depend on a stable interface without pulling in a metrics SDK.

Changes:

• Introduces observability module with metrics traits (SearchMetrics, LookupTableMetrics, NetworkMetrics) plus a composed NodeMetrics surface and unimock-generated doubles.
• Adds closed-enum label types (SearchOutcome, MessageType, LevelBucket) with LevelBucket bucketing logic and tests for boundary behavior.
• Promotes core::model::direction module visibility so Direction can be used in public metrics trait signatures and re-exported via observability.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/observability/mod.rs	Defines the public observability module surface and re-exports traits/labels (and Direction).
src/observability/metrics.rs	Adds backend-free metrics traits with unimock doubles and a composite NodeMetrics trait.
src/observability/labels.rs	Adds bounded label enums and LevelBucket bucketing to keep label cardinality constrained.
src/observability/labels_test.rs	Tests bucket mapping across boundaries and overflow behavior.
src/lib.rs	Exposes observability as a public module from the crate root.
src/core/model/mod.rs	Makes direction module public to support public trait signatures.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[S T (staheri14)]

Claude (@claude) review this PR

[claude]

Claude finished S T (@staheri14)'s task —— View job

---

Review: PR #123 — metrics interface

Nice, clean interface-only PR. The design is well thought out: infallibility + cardinality contracts documented in the module header, closed-enum labels, and a blanket-impl NodeMetrics composite so backends only implement the three sub-traits. Tests cover the bucket boundaries thoroughly. Below is my feedback — nothing blocking.

Strengths

• NodeMetrics blanket impl (metrics.rs:33) is the right call — backends implement the three sub-traits and the composite follows automatically. Well documented.
• LevelBucket::from_level (labels.rs:51) is const, #[must_use], and total over all usize, with From delegating to it. The test at labels_test.rs:7 checks every boundary (15/16, 63/64, 255/256, usize::MAX) and asserts From agrees with from_level.
• Bounded-label-by-construction approach nicely enforces the cardinality contract at the type level.

Suggestions (non-blocking)

1. MessageType only covers 3 of the ~13 Event variants (labels.rs:15). The real Event enum (network/mod.rs:14-37) has GetMaxLevelOp, RetMaxLevelOp, GetNeighborOp, RetNeighborOp, GetLinkOp, SetLinkOp, BuddyOp, CheckNeighborOp, etc. When wiring lands ([Obs] Instrument: core search operations via metrics interface #112–[Obs] Instrument: network send/receive paths #114), record_message_sent/received won't be able to label most traffic. Worth confirming this is intentional for the interface-only stage, and that the follow-ups will extend the enum. Also consider whether TestMessage belongs in a production metric label — emitting it in prod builds is a (small) cardinality/noise concern.

2. Missing derives on the label enums. SearchOutcome, MessageType, LevelBucket derive Debug, Clone, Copy, PartialEq, Eq but not Hash. Most metrics backends (e.g. Prometheus label maps) key on labels, so Hash — and likely PartialOrd/Ord for stable ordering — will probably be needed by the backend in [Obs] Implement: OTel metrics & Prometheus exporter #110. Direction already derives Hash; the new enums being used in the same trait signatures don't. Adding them now keeps the public interface stable.

3. SearchMetrics::record_search takes hops: usize (metrics.rs:9) as a raw unbounded value. hops looks like it feeds a histogram rather than a label, so this is fine — but a doc note clarifying "recorded as a distribution, not a label" would reinforce the cardinality contract and prevent a future reader from treating it as a label.

4. Naming consistency: the module doc says labels are "closed enums only", and hops/elapsed are values not labels — the distinction between label args and value args isn't obvious from the signatures alone. A one-line convention note in metrics.rs (which params are labels vs. observation values) would help implementers honor the cardinality contract.

5. Minor: consider re-exporting the *Mock types under #[cfg(test)] or a feature gate if they shouldn't ship in release builds — right now mod.rs:27-30 re-exports them unconditionally. If consumers need them in their own tests, unconditional is correct; just flagging the intent.

Verification

I couldn't run cargo build/clippy/test in this environment (command not permitted), so I reviewed statically. Please confirm CI is green — in particular that the unimock api = ... codegen and the pub promotion of direction compile cleanly.

Overall: solid, well-documented interface. The main thing I'd resolve before wiring is the MessageType coverage and the missing Hash/Ord derives, since those affect the public surface backends will build on.
• sanaz/obs-metrics-interface

[S T (staheri14)]

Thanks for the review. Dispositions:

1. MessageType coverage: deferred to the network-instrumentation work, where Event is actually mapped to labels. Nothing records the missing 8 variants until then, so extending the enum now would be premature. Added a note on the enum so the gap is explicit (b100cdc). TestMessage stays for now to mirror Event; it just isn't emitted on the search paths.

2. Hash/Ord derives: done, added Hash, PartialOrd, Ord to SearchOutcome, MessageType, and LevelBucket (7e66436).

3 & 4. hops distribution note + label/value convention: documented both (a1aa128): a module-level convention note in metrics.rs (closed-enum params are labels; scalar params are distribution values) plus a line on record_search noting hops/elapsed are recorded as distributions, not labels.

5. Gating the *Mock re-exports: keeping them ungated to match the existing NetworkMock (also always-compiled). #[cfg(test)] would actually make the doubles unavailable to downstream crates' tests (that cfg applies only to this crate's own tests), so unconditional is the right call for consumers.