feat(acp-nats): add OpenTelemetry metrics to initialize handler

[yordis]

Summary

Adds OpenTelemetry metrics to the acp-nats initialize handler. Records request duration and success/failure for each initialize call.

Changes

• Dependencies: opentelemetry, opentelemetry_sdk, trogon-std
• Telemetry module: Metrics with record_request (requests_total, request_duration)
• Bridge: Now accepts clock: C (GetElapsed) and meter: &Meter for testable timing and metrics
• Initialize handler: Records metrics via bridge.clock.now() / bridge.clock.elapsed() and bridge.metrics.record_request()
• Tests: initialize_records_metrics_on_success and initialize_records_metrics_on_failure verify metrics are captured

Verification

cargo build -p acp-nats
cargo test -p acp-nats

All 20 tests pass.

[cursor]

PR Summary

Medium Risk
Touches the core Bridge constructor/generics and initialize request flow, which can ripple to downstream call sites and affects runtime observability behavior (new metrics emission), but does not change request semantics.

Overview
Adds OpenTelemetry request metrics to acp-nats’s initialize path by introducing a telemetry::Metrics helper that records acp.request.count and acp.request.duration with method/success attributes.

Refactors Bridge to carry an injectable clock (trogon_std::time::GetElapsed) and an OpenTelemetry Meter so the initialize handler can time each request and emit success/failure metrics, with new in-memory exporter tests verifying both outcomes.

^{Written by Cursor Bugbot for commit d5f373f. This will update automatically on new commits. Configure here.}

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0e390cb and d5f373f.

⛔ Files ignored due to path filters (1)

• rsworkspace/Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (6)

• rsworkspace/crates/acp-nats/Cargo.toml
• rsworkspace/crates/acp-nats/src/agent/initialize.rs
• rsworkspace/crates/acp-nats/src/agent/mod.rs
• rsworkspace/crates/acp-nats/src/lib.rs
• rsworkspace/crates/acp-nats/src/telemetry/metrics.rs
• rsworkspace/crates/acp-nats/src/telemetry/mod.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• rsworkspace/crates/acp-nats/src/lib.rs
• rsworkspace/crates/acp-nats/Cargo.toml

---

Walkthrough

Adds OpenTelemetry-based telemetry and a clock abstraction to acp-nats: new dependencies, a Metrics helper and telemetry module, Bridge gains clock and metrics, initialize::handle measures elapsed time and records metrics, and tests updated for metrics and clock wiring.

Changes

Cohort / File(s)	Summary
Manifest rsworkspace/crates/acp-nats/Cargo.toml	Added dependencies: opentelemetry = "0.31.0", trogon-std = { path = "../trogon-std" }; added dev-deps: opentelemetry_sdk = { version = "0.31.0", features = ["rt-tokio","metrics","testing"] }, trogon-std = { path = "../trogon-std", features = ["test-support"] }.
Bridge / Agent rsworkspace/crates/acp-nats/src/agent/mod.rs	Made Bridge generic over a clock C: GetElapsed; added clock: C and metrics: Metrics fields; updated Bridge::new to accept clock and meter; adjusted Agent impl and tests to construct Bridge with clock and meter.
Initialize handler rsworkspace/crates/acp-nats/src/agent/initialize.rs	handle now generic over C: GetElapsed and accepts Bridge<N, C>; captures start time via bridge.clock.now(); records elapsed and success via bridge.metrics.record_request(...); tests updated to assert metrics on success/failure.
Telemetry module wiring rsworkspace/crates/acp-nats/src/lib.rs, rsworkspace/crates/acp-nats/src/telemetry/mod.rs	Added pub(crate) mod telemetry; and pub(crate) mod metrics; to expose telemetry internals within the crate.
Metrics implementation rsworkspace/crates/acp-nats/src/telemetry/metrics.rs	Added Metrics (#[derive(Clone)]) with Counter<u64> and Histogram<f64> instruments; Metrics::new(meter: &Meter) constructs instruments; record_request(&self, method, duration, success) records count and duration with attributes.
Tests & test helpers rsworkspace/crates/acp-nats/src/agent/...	Updated tests and mock_bridge to use SystemClock and a Meter (opentelemetry in-memory SDK) and to verify metrics recording; test helpers import trogon_std::time::GetElapsed.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant Bridge
  participant NATS as NATS_Client
  participant Meter as Meter/Exporter

  rect rgba(200,230,255,0.5)
    Client->>Bridge: initialize request (args)
    Bridge->>Bridge: start = clock.now()
    Bridge->>NATS_Client: request initialize (await)
    alt success
      NATS_Client-->>Bridge: response
      Bridge->>Meter: metrics.record_request("initialize", elapsed, true)
      Bridge-->>Client: Ok(response)
    else failure
      NATS_Client-->>Bridge: error
      Bridge->>Meter: metrics.record_request("initialize", elapsed, false)
      Bridge-->>Client: Err(error)
    end
  end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• feat(acp-nats): minimal initialize-only ACP-NATS bridge #10: Modifies the same acp-nats Agent/Bridge and initialize handler; closely related to the clock/metrics generics added here.
• feat(trogon-nats): add trogon-nats pkg #4: Adds trogon-related crates/mocks and testing utilities used by the updated tests (AdvancedMockNatsClient, SystemClock, etc.).

Poem

🐰 I tapped my paws to start the clock,

I counted hops and timed each knock,
Requests now sing in measured beats,
Counters climb and histograms meet,
A joyful rabbit tracks each clocked-rock!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 31.25% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and concisely summarizes the main change: adding OpenTelemetry metrics to the initialize handler in acp-nats.
Description check	✅ Passed	The description is directly related to the changeset, covering dependencies, telemetry module, Bridge updates, handler changes, and test additions with verification steps.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch feat/acp-nats-metrics

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

rsworkspace/crates/acp-nats/Cargo.toml (1)

9-9: Move opentelemetry_sdk with testing feature to dev-dependencies.

opentelemetry_sdk is used exclusively within the #[cfg(test)] mod tests block in initialize.rs, specifically for creating test fixtures (mock_bridge_with_metrics()). The testing feature enables InMemoryMetricExporter, which is test-only. Keeping this in [dependencies] unnecessarily inflates non-test builds.

Suggested Cargo.toml adjustment

 [dependencies]
 opentelemetry = "0.31.0"
-opentelemetry_sdk = { version = "0.31.0", features = ["rt-tokio", "metrics", "testing"] }
+opentelemetry_sdk = { version = "0.31.0", features = ["rt-tokio", "metrics"] }
 async-nats = "0.45.0"
 async-trait = "0.1.89"
 serde = { version = "1.0.228", features = ["derive"] }
 serde_json = "1.0.149"
 tokio = { version = "1.49.0", features = ["rt", "macros", "sync", "time"] }
 tracing = "0.1.44"
 
 trogon-nats = { path = "../trogon-nats" }
 trogon-std = { path = "../trogon-std" }
 
 [dev-dependencies]
+opentelemetry_sdk = { version = "0.31.0", features = ["rt-tokio", "metrics", "testing"] }
 trogon-nats = { path = "../trogon-nats", features = ["test-support"] }
 trogon-std = { path = "../trogon-std", features = ["test-support"] }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@rsworkspace/crates/acp-nats/Cargo.toml` at line 9, Move the opentelemetry_sdk
dependency (with the "testing" feature) from regular [dependencies] to
[dev-dependencies] so it is only compiled for tests; update Cargo.toml to remove
opentelemetry_sdk from the main dependencies section and add it under
[dev-dependencies] with features = ["rt-tokio","metrics","testing"] to preserve
InMemoryMetricExporter used by the #[cfg(test)] tests in initialize.rs (see
mock_bridge_with_metrics and the tests module) so non-test builds are not
inflated.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@rsworkspace/crates/acp-nats/src/agent/initialize.rs`:
- Around line 205-229: Update the two tests
initialize_records_metrics_on_success and initialize_records_metrics_on_failure
to assert exact metric instruments and attributes rather than just non-empty
export: after calling bridge.initialize(...) and
force_flush()/get_finished_metrics(), locate the metric(s) emitted by the
instrument that records RPC latency/count (use the same metric name seen in
exporter output), then assert there exists a datapoint whose "method" attribute
equals "acp.agent.initialize" and whose "success" attribute is true in the
success test and false in the failure test; use exporter.get_finished_metrics()
results to filter by metric name and iterate datapoints to check attributes
(refer to mock_bridge_with_metrics, exporter, provider, and bridge.initialize to
find test scaffolding and metric emission points).

---

Nitpick comments:
In `@rsworkspace/crates/acp-nats/Cargo.toml`:
- Line 9: Move the opentelemetry_sdk dependency (with the "testing" feature)
from regular [dependencies] to [dev-dependencies] so it is only compiled for
tests; update Cargo.toml to remove opentelemetry_sdk from the main dependencies
section and add it under [dev-dependencies] with features =
["rt-tokio","metrics","testing"] to preserve InMemoryMetricExporter used by the
#[cfg(test)] tests in initialize.rs (see mock_bridge_with_metrics and the tests
module) so non-test builds are not inflated.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cfdea48 and 7b84012.

⛔ Files ignored due to path filters (1)

• rsworkspace/Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (6)

• rsworkspace/crates/acp-nats/Cargo.toml
• rsworkspace/crates/acp-nats/src/agent/initialize.rs
• rsworkspace/crates/acp-nats/src/agent/mod.rs
• rsworkspace/crates/acp-nats/src/lib.rs
• rsworkspace/crates/acp-nats/src/telemetry/metrics.rs
• rsworkspace/crates/acp-nats/src/telemetry/mod.rs

[coderabbitai]

♻️ Duplicate comments (1)

rsworkspace/crates/acp-nats/src/agent/initialize.rs (1)

205-243: ⚠️ Potential issue | 🟡 Minor

Strengthen assertions in metrics tests (current checks are too permissive).

Line 220 and Line 238 only validate non-empty exports, so these tests can pass even if the wrong metric/attributes are emitted. Please assert expected instruments and method/success attributes explicitly for success and failure cases.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@rsworkspace/crates/acp-nats/src/agent/initialize.rs` around lines 205 - 243,
Update the two tests initialize_records_metrics_on_success and
initialize_records_metrics_on_failure to assert the exact metric and attribute
values instead of only checking non-empty exports: after calling
bridge.initialize and force_flush, inspect exporter.get_finished_metrics() for a
metric with the expected instrument/metric name emitted by
mock_bridge_with_metrics (e.g., the initialize duration counter/histogram) and
assert that it contains attributes method="initialize" and success="true" in the
success test and success="false" in the failure test; keep using
mock_bridge_with_metrics, bridge.initialize, exporter.get_finished_metrics and
provider.force_flush()/shutdown() but add explicit checks on metric name and
attribute key/value pairs rather than just non-empty.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@rsworkspace/crates/acp-nats/src/agent/initialize.rs`:
- Around line 205-243: Update the two tests
initialize_records_metrics_on_success and initialize_records_metrics_on_failure
to assert the exact metric and attribute values instead of only checking
non-empty exports: after calling bridge.initialize and force_flush, inspect
exporter.get_finished_metrics() for a metric with the expected instrument/metric
name emitted by mock_bridge_with_metrics (e.g., the initialize duration
counter/histogram) and assert that it contains attributes method="initialize"
and success="true" in the success test and success="false" in the failure test;
keep using mock_bridge_with_metrics, bridge.initialize,
exporter.get_finished_metrics and provider.force_flush()/shutdown() but add
explicit checks on metric name and attribute key/value pairs rather than just
non-empty.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7b84012 and 0e390cb.

⛔ Files ignored due to path filters (1)

• rsworkspace/Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (6)

• rsworkspace/crates/acp-nats/Cargo.toml
• rsworkspace/crates/acp-nats/src/agent/initialize.rs
• rsworkspace/crates/acp-nats/src/agent/mod.rs
• rsworkspace/crates/acp-nats/src/lib.rs
• rsworkspace/crates/acp-nats/src/telemetry/metrics.rs
• rsworkspace/crates/acp-nats/src/telemetry/mod.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• rsworkspace/crates/acp-nats/Cargo.toml
• rsworkspace/crates/acp-nats/src/telemetry/metrics.rs

[cursor]

Cursor Bugbot has reviewed your changes and found 2 potential issues.