feat(acp-nats): align metrics with OTEL SemConv naming#32
Merged
Conversation
PR SummaryMedium Risk Overview Adds a new Written by Cursor Bugbot for commit f80c975. This will update automatically on new commits. Configure here. |
WalkthroughRenames ACP metrics from Changes
Sequence Diagram(s)sequenceDiagram
participant Client as "Client"
participant Handler as "NewSessionHandler"
participant Bridge as "Bridge"
participant Metrics as "Metrics"
participant Exporter as "Telemetry Exporter"
Client->>Handler: send NewSession request
Handler->>Bridge: process NewSession
alt NewSession success
Bridge->>Metrics: record_request(duration, success)
Bridge->>Metrics: record_session_created()
Metrics->>Exporter: increment `acp.sessions.created`
Metrics->>Exporter: increment `acp.requests`/record duration
else failure
Bridge->>Metrics: record_error()
Metrics->>Exporter: increment `acp.errors`
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
|
Code Coverage SummaryDetailsDiff against mainResults for commit: f80c975 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
rsworkspace/crates/acp-nats/src/telemetry/metrics.rs (1)
83-158: Consider strengthening test assertions to verify actual metric names.The current tests only verify that metrics are non-empty after recording. They don't confirm that the correct metric names (
acp.requests,acp.errors,acp.sessions.created) were actually emitted. This gap is covered by the handler-level tests, but unit tests here could provide stronger guarantees.Example: Verify metric name in test_metrics_record_session_created
#[tokio::test] async fn test_metrics_record_session_created() { let (metrics, exporter, provider) = create_test_metrics(); metrics.record_session_created(); provider.force_flush().unwrap(); let finished_metrics = exporter.get_finished_metrics().unwrap(); - assert!(!finished_metrics.is_empty()); + assert!(!finished_metrics.is_empty()); + let has_sessions_metric = finished_metrics + .iter() + .flat_map(|rm| rm.scope_metrics()) + .flat_map(|sm| sm.metrics()) + .any(|m| m.name() == "acp.sessions.created"); + assert!(has_sessions_metric, "expected acp.sessions.created metric"); provider.shutdown().unwrap(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-nats/src/telemetry/metrics.rs` around lines 83 - 158, Update the tests to assert that the expected metric names are present in the exported metrics rather than only checking non-empty; for each test (test_metrics_record_request_success, test_metrics_record_request_failure, test_metrics_record_session_created, test_metrics_record_error, test_metrics_multiple_recordings) iterate finished_metrics returned by exporter.get_finished_metrics() and assert that at least one metric has the expected name (e.g. "acp.requests" for record_request calls, "acp.errors" for record_error, "acp.sessions.created" for record_session_created), keeping the existing force_flush()/provider.shutdown() usage.rsworkspace/crates/acp-nats/src/agent/new_session.rs (1)
85-85: Assert the newacp.sessions.createdside effect in this handler.Line 85 is the behavior added by this PR, but the tests in this file still only verify
acp.requests/acp.errors. If this call is dropped later, theMetricsunit tests can still pass while thenew_sessionpath regresses silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-nats/src/agent/new_session.rs` at line 85, The new_session handler now calls bridge.metrics.record_session_created(), but the tests in this file only assert acp.requests/acp.errors; update the existing new_session tests to also assert the acp.sessions.created side effect by checking that bridge.metrics.record_session_created was called (or that the acp.sessions.created metric was emitted/incremented) after invoking the handler; locate usages of bridge.metrics.record_session_created in new_session.rs and add an assertion in the corresponding test(s) that inspects the metrics sink/mock (or the published acp.sessions.created event) to ensure the call happened.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@rsworkspace/crates/acp-nats/src/agent/new_session.rs`:
- Line 85: The new_session handler now calls
bridge.metrics.record_session_created(), but the tests in this file only assert
acp.requests/acp.errors; update the existing new_session tests to also assert
the acp.sessions.created side effect by checking that
bridge.metrics.record_session_created was called (or that the
acp.sessions.created metric was emitted/incremented) after invoking the handler;
locate usages of bridge.metrics.record_session_created in new_session.rs and add
an assertion in the corresponding test(s) that inspects the metrics sink/mock
(or the published acp.sessions.created event) to ensure the call happened.
In `@rsworkspace/crates/acp-nats/src/telemetry/metrics.rs`:
- Around line 83-158: Update the tests to assert that the expected metric names
are present in the exported metrics rather than only checking non-empty; for
each test (test_metrics_record_request_success,
test_metrics_record_request_failure, test_metrics_record_session_created,
test_metrics_record_error, test_metrics_multiple_recordings) iterate
finished_metrics returned by exporter.get_finished_metrics() and assert that at
least one metric has the expected name (e.g. "acp.requests" for record_request
calls, "acp.errors" for record_error, "acp.sessions.created" for
record_session_created), keeping the existing force_flush()/provider.shutdown()
usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c061e0c-e4d0-4cd4-980b-6e282edf980c
📒 Files selected for processing (10)
rsworkspace/crates/acp-nats/src/agent/authenticate.rsrsworkspace/crates/acp-nats/src/agent/cancel.rsrsworkspace/crates/acp-nats/src/agent/ext_method.rsrsworkspace/crates/acp-nats/src/agent/ext_notification.rsrsworkspace/crates/acp-nats/src/agent/initialize.rsrsworkspace/crates/acp-nats/src/agent/load_session.rsrsworkspace/crates/acp-nats/src/agent/new_session.rsrsworkspace/crates/acp-nats/src/agent/prompt.rsrsworkspace/crates/acp-nats/src/agent/set_session_mode.rsrsworkspace/crates/acp-nats/src/telemetry/metrics.rs
- Rename acp.request.count -> acp.requests (counters must not use count) - Rename acp.errors.total -> acp.errors (counters must not use _total) - Add acp.sessions.created counter and record_session_created() - Add unit tests for Metrics struct - Update all handler test assertions to use new metric names Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis
force-pushed
the
feat/acp-nats-metrics
branch
from
83b08a4 to
f80c975
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
rsworkspace/crates/acp-nats/src/agent/new_session.rs (1)
81-85: Cover the new session counter at the call site.
record_session_created()is the new behavior in this PR, but the surrounding tests still only assertacp.requests/acp.errors. Please add a success-path assertion foracp.sessions.createdand a failure-path negative check so this wiring cannot regress silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-nats/src/agent/new_session.rs` around lines 81 - 85, Tests need to assert the new session counter is updated at the call site where bridge.metrics.record_session_created() is invoked; update the success-path tests that hit the Ok branch (the code around new_session.rs where result is matched and record_session_created() is called) to assert that the metric "acp.sessions.created" increments (or equals expected value) after a successful create, and update the failure-path tests that exercise the Err branch to assert that "acp.sessions.created" does not increment (negative check). Ensure you reference the bridge.metrics.record_session_created() call when adding these assertions so the wiring cannot regress.rsworkspace/crates/acp-nats/src/telemetry/metrics.rs (2)
83-157: Assert the concrete metric contract in these tests.These tests only check
finished_metrics.is_empty(). They still pass ifrecord_request()only emits the histogram, or ifrecord_session_created()increments the wrong counter. Please assert the emitted metric names/data points (acp.requests,acp.request.duration,acp.sessions.created,acp.errors) so the rename is actually protected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-nats/src/telemetry/metrics.rs` around lines 83 - 157, Update the tests to assert the concrete metric names and datapoints instead of only checking finished_metrics.is_empty(): after calling create_test_metrics() and invoking metrics.record_request / record_session_created / record_error (e.g., in test_metrics_record_request_success, test_metrics_record_request_failure, test_metrics_record_session_created, test_metrics_record_error, test_metrics_multiple_recordings) call provider.force_flush() and then inspect exporter.get_finished_metrics() to verify it contains metrics with the expected instrument names "acp.requests", "acp.request.duration", "acp.sessions.created", and "acp.errors" and that their datapoints reflect the recorded values (counter increments for requests/sessions/errors and a histogram/summary with the recorded duration for request.duration). Use the exporter/finished metric objects’ name and datapoint fields to assert exact counts and duration values for each specific test so renames or missing emissions would fail.
15-31: Coordinate the observability rollout with this rename.Removing
acp.request.countandacp.errors.totaloutright will blank any dashboards, alerts, or collector transforms that still query the old names. Please ship the matching observability config updates with this rollout, or keep a short-lived compatibility mapping during the cutover.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rsworkspace/crates/acp-nats/src/telemetry/metrics.rs` around lines 15 - 31, The diff renames metrics (new instruments requests, errors) which will break dashboards expecting acp.request.count and acp.errors.total; either update the observability configs (dashboards/collector transforms/alerts) to use the new names or add short-lived compatibility metrics by also creating instruments with the old names (e.g., build u64_counter("acp.request.count") and u64_counter("acp.errors.total") and increment them alongside requests and errors in the same code paths) so both sets exist during the cutover; ensure the legacy instruments are clearly marked for removal and coordinate the rollout with the observability team.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@rsworkspace/crates/acp-nats/src/agent/new_session.rs`:
- Around line 81-85: Tests need to assert the new session counter is updated at
the call site where bridge.metrics.record_session_created() is invoked; update
the success-path tests that hit the Ok branch (the code around new_session.rs
where result is matched and record_session_created() is called) to assert that
the metric "acp.sessions.created" increments (or equals expected value) after a
successful create, and update the failure-path tests that exercise the Err
branch to assert that "acp.sessions.created" does not increment (negative
check). Ensure you reference the bridge.metrics.record_session_created() call
when adding these assertions so the wiring cannot regress.
In `@rsworkspace/crates/acp-nats/src/telemetry/metrics.rs`:
- Around line 83-157: Update the tests to assert the concrete metric names and
datapoints instead of only checking finished_metrics.is_empty(): after calling
create_test_metrics() and invoking metrics.record_request /
record_session_created / record_error (e.g., in
test_metrics_record_request_success, test_metrics_record_request_failure,
test_metrics_record_session_created, test_metrics_record_error,
test_metrics_multiple_recordings) call provider.force_flush() and then inspect
exporter.get_finished_metrics() to verify it contains metrics with the expected
instrument names "acp.requests", "acp.request.duration", "acp.sessions.created",
and "acp.errors" and that their datapoints reflect the recorded values (counter
increments for requests/sessions/errors and a histogram/summary with the
recorded duration for request.duration). Use the exporter/finished metric
objects’ name and datapoint fields to assert exact counts and duration values
for each specific test so renames or missing emissions would fail.
- Around line 15-31: The diff renames metrics (new instruments requests, errors)
which will break dashboards expecting acp.request.count and acp.errors.total;
either update the observability configs (dashboards/collector transforms/alerts)
to use the new names or add short-lived compatibility metrics by also creating
instruments with the old names (e.g., build u64_counter("acp.request.count") and
u64_counter("acp.errors.total") and increment them alongside requests and errors
in the same code paths) so both sets exist during the cutover; ensure the legacy
instruments are clearly marked for removal and coordinate the rollout with the
observability team.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc767290-f871-4859-8ad4-8de54b6937ba
📒 Files selected for processing (10)
rsworkspace/crates/acp-nats/src/agent/authenticate.rsrsworkspace/crates/acp-nats/src/agent/cancel.rsrsworkspace/crates/acp-nats/src/agent/ext_method.rsrsworkspace/crates/acp-nats/src/agent/ext_notification.rsrsworkspace/crates/acp-nats/src/agent/initialize.rsrsworkspace/crates/acp-nats/src/agent/load_session.rsrsworkspace/crates/acp-nats/src/agent/new_session.rsrsworkspace/crates/acp-nats/src/agent/prompt.rsrsworkspace/crates/acp-nats/src/agent/set_session_mode.rsrsworkspace/crates/acp-nats/src/telemetry/metrics.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- rsworkspace/crates/acp-nats/src/agent/initialize.rs
- rsworkspace/crates/acp-nats/src/agent/cancel.rs
- rsworkspace/crates/acp-nats/src/agent/load_session.rs
- rsworkspace/crates/acp-nats/src/agent/ext_notification.rs
- rsworkspace/crates/acp-nats/src/agent/prompt.rs
yordis
added a commit
that referenced
this pull request
Mar 13, 2026
Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Aligns acp-nats metrics with OpenTelemetry Semantic Conventions:
count; pluralize the entity)_total)record_session_created()Rationale
OTEL SemConv explicitly forbids
_totalon counters (confusing in delta backends) and reservescountfor limit-type instruments. For counters of discrete items, pluralization is preferred (e.g.system.paging.faults,system.network.packets).Files changed
telemetry/metrics.rs— full replacement (renames, new counter, unit tests)agent/*.rs— test assertion updates across 9 handler filesagent/new_session.rs— addrecord_session_created()call in success pathValidation
cargo build -p acp-nats cargo test -p acp-nats --libAll 440 tests pass.