You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The PR adds OTLP metrics export and mock collection, instruments WebSocket activity, makes direct-message queuing fallible, narrows analysis scopes, and standardizes Clippy expectations and panic handling across tests, generators, and build utilities.
Changes AgentRegistry::queue_direct to return Option when recipient is not found, enabling graceful handling of unknown agents.
WebSocket connection and message metrics services/ws/src/lib.rs
Initializes lazy-loaded MESSAGES_RECEIVED counter and ACTIVE_CONNECTIONS gauge, and instruments connection lifecycle and inbound message processing.
Server OTLP integration and test helpers services/ws-server/src/main.rs, services/ws-test-server/..., services/ws-test-server/tests/...
Propagates OTLP initialization errors in server entry points, exports reusable WebSocket connect/payload helpers, and adds end-to-end telemetry integration tests.
Tooling configuration and lint-policy standardization
Layer / File(s)
Summary
Analysis scope and Clippy policy .deepsource.toml, .mise/config.*.toml, config/clippy.toml, config/ast-grep/rules/*
Narrows DeepSource to exclude only generated paths, filters lcov coverage by path prefix, limits link-check to source directories, defines test-allowed Clippy patterns, bans certain methods, and forbids mod items in test files.
Edge-toolkit sanctioned-use and test lint cleanup libs/edge-toolkit/src/config.rs, libs/edge-toolkit/src/ws.rs, libs/edge-toolkit/tests/*
Applies targeted lint expectations to sanctioned project-root access and JSON-schema conversions; removes broad test suppressions and replaces custom expect messages with unwrap.
Test lint and panic-handling standardization libs/path/tests/*, libs/test-helpers/src/lib.rs, libs/ws-runner-common/tests/*, services/modules/tests/*, services/storage/tests/*, services/ws-modules/*/..., services/ws-pyo3-runner/tests/*, services/ws-wasi-runner/tests/*, services/ws/tests/*, utilities/cli/tests/*
Removes expect-related suppressions and standardizes panic handling via unwrap, unwrap_err, or direct indexing across all affected test modules and shared libraries.
We reviewed changes in 18dc4f9...26a63ce on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.
Some issues found as part of this review are outside of the diff in this pull request and aren't shown in the inline review comments due to GitHub's API limitations. You can see those issues on the DeepSource dashboard.
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
Duplicate dual-encoding decode logic between handle_traces and handle_metrics.
handle_metrics (Lines 258-281) repeats the same is_protobuf detection + protobuf/JSON decode + response-encoding-echo pattern already in handle_traces (Lines 213-241). Given this is a small, low-churn test-support crate, extracting a shared generic helper is optional but would reduce copy/paste drift if a third endpoint type is ever added.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/otlp-mock/src/lib.rs` around lines 253 - 282, Reduce the duplicated
dual-encoding logic between handle_traces and handle_metrics by extracting and
reusing a shared helper for content-type detection, protobuf/JSON decoding, and
response encoding. Keep each handler responsible only for endpoint-specific
request storage and use the helper for both existing endpoint types.
Global providers are set before all fallible builds complete.
opentelemetry::global::set_tracer_provider(...) (Line 102) and set_meter_provider(...) (Line 119) run before the metric exporter build, log exporter build, EnvFilter::try_new, and set_global_default calls that can still fail with ?. If any of those later steps errors, init returns Err, but the tracer/meter providers have already been registered as process-wide globals and never get shut down (they aren't returned in an OtelHandles, so no shutdown() call is possible). In practice every current caller (ws-server, ws-wasi-runner, test helpers) treats init's error as fatal and exits shortly after, which limits exposure, but the leaked global registration is still an avoidable inconsistency and would bite any caller that retries init or continues after a soft failure.
Consider building all exporters/providers first and only calling set_tracer_provider/set_meter_provider once every fallible step has succeeded, right before Ok(OtelHandles { ... }).
♻️ Sketch of reordering the global registration
let tracer_provider = SdkTracerProvider::builder()
.with_batch_exporter(span_exporter)
.with_resource(resource.clone())
.build();
- // Set the global tracer provider so direct `global::tracer(...)` spans (e.g. the ws hub's `ws.connect`)- // export too -- not just the `tracing`-subscriber spans routed through the layer below.- opentelemetry::global::set_tracer_provider(tracer_provider.clone());-
let otel_tracing_layer = OpenTelemetryLayer::new(tracer_provider.tracer(config.service_label.clone()));
// Metrics ride the same OTLP/HTTP transport as spans and logs, posting to `<collector_url>/metrics`.
let metric_endpoint = format!("{}/metrics", config.collector_url);
let metric_exporter = MetricExporter::builder()
.with_http()
.with_protocol(protocol)
.with_endpoint(metric_endpoint)
.with_headers(headers.clone())
.build()?;
let meter_provider = SdkMeterProvider::builder()
.with_periodic_exporter(metric_exporter)
.with_resource(resource.clone())
.build();
- opentelemetry::global::set_meter_provider(meter_provider.clone());
let log_directives = std::env::var(RUST_LOG).unwrap_or_else(|_| "info".to_string());
let env_filter = EnvFilter::try_new(log_directives)?;
@@
set_global_default(subscriber)?;
++ // Only register global providers once every fallible step above has succeeded.+ opentelemetry::global::set_tracer_provider(tracer_provider.clone());+ opentelemetry::global::set_meter_provider(meter_provider.clone());
Ok(OtelHandles {
tracer_provider,
logger_provider,
meter_provider,
})
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/et-otlp/src/lib.rs` around lines 100 - 129, Move the global provider
registrations out of the early setup in init and place set_tracer_provider and
set_meter_provider only after all fallible exporter construction, EnvFilter
creation, and global subscriber setup have succeeded, immediately before
returning Ok(OtelHandles { ... }). Keep the locally built providers available
for layer construction and handle creation without registering process-wide
globals on any error path.
Stale wording in the lint justification: "expect calls" should read "unwrap calls."
The #[expect(clippy::unwrap_used, ...)] reason at line 49 still says "the only way these expect calls fire is a serde_json bug", but the code below no longer uses .expect() — it uses .unwrap(). Leftover from before this refactor.
✏️ Fix wording
- reason = "all conversions are between serde-derived types; the only way these expect calls fire is a serde_json bug"+ reason = "all conversions are between serde-derived types; the only way these unwrap calls fire is a serde_json bug"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@utilities/int-gen/src/openapi.rs` around lines 47 - 50, Update the reason
text on the clippy::unwrap_used #[expect] attribute to refer to “unwrap calls”
instead of “expect calls,” leaving the lint configuration and conversion
behavior unchanged.
This dump() implementation is identical to services/ws-modules/wasi-data1/src/coverage.rs except for the output filename. As per coding guidelines, shared low-dependency helpers should live in a shared test-support crate rather than being copy-pasted per module.
♻️ Suggested consolidation
// et-test-helpers (or a shared coverage-support crate)pubfndump_coverage(path:&str){letmut coverage = Vec::new();// SAFETY: single-threaded guest; capture_coverage reads the instrumented counters once at run() end.unsafe{
minicov::capture_coverage(&mut coverage).unwrap();}
fs_err::write(path, coverage).unwrap();}
Then each module calls dump_coverage("/cov/et_ws_wasi_comm1.profraw").
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/ws-modules/wasi-comm1/src/coverage.rs` around lines 9 - 16, Move the
duplicated coverage-capture and file-writing logic from dump() into a shared
low-dependency test-support or coverage-support helper named dump_coverage(path:
&str). Update this module’s dump() to call dump_coverage with its existing
output path, and apply the same reuse in the corresponding wasi-data1 coverage
module.
Source: Coding guidelines
services/ws-test-server/src/lib.rs (1)
108-137: 🚀 Performance & Scalability | 🔵 Trivial
Consider a type alias for the repeated stream type.
tokio_tungstenite::WebSocketStream<tokio_tungstenite::MaybeTlsStream<tokio::net::TcpStream>> is spelled out three times across connect_agent/next_payload. A pub type WsStream = ... alias would cut noise without behavior change.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/ws-test-server/src/lib.rs` around lines 108 - 137, Introduce a
public type alias for the repeated WebSocket stream type, then update the
signatures of connect_agent and next_payload to use the alias instead of the
fully qualified type. Preserve all existing behavior and visibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@services/ws-modules/face-detection/src/test_face_detection.rs`:
- Around line 4-5: Replace the indexed assertions in the face-detection tests
with bounds-safe assertion patterns, covering filtered[0], filtered[1],
large[1], priors[0][0..3], and decoded[0..3]. Preserve the existing expected
values and ordering while avoiding indexing and slicing operations that trigger
clippy::indexing_slicing.
In `@services/ws-test-server/src/lib.rs`:
- Around line 84-106: Update connect_agent’s et-connect-ack wait loop to enforce
the same 5-second timeout used by next_payload. Wrap each stream.next().await
wait, preserve successful ack handling, and fail promptly with the existing
panic path when the timeout expires.
---
Nitpick comments:
In `@libs/et-otlp/src/lib.rs`:
- Around line 100-129: Move the global provider registrations out of the early
setup in init and place set_tracer_provider and set_meter_provider only after
all fallible exporter construction, EnvFilter creation, and global subscriber
setup have succeeded, immediately before returning Ok(OtelHandles { ... }). Keep
the locally built providers available for layer construction and handle creation
without registering process-wide globals on any error path.
In `@libs/otlp-mock/src/lib.rs`:
- Around line 253-282: Reduce the duplicated dual-encoding logic between
handle_traces and handle_metrics by extracting and reusing a shared helper for
content-type detection, protobuf/JSON decoding, and response encoding. Keep each
handler responsible only for endpoint-specific request storage and use the
helper for both existing endpoint types.
In `@services/ws-modules/wasi-comm1/src/coverage.rs`:
- Around line 9-16: Move the duplicated coverage-capture and file-writing logic
from dump() into a shared low-dependency test-support or coverage-support helper
named dump_coverage(path: &str). Update this module’s dump() to call
dump_coverage with its existing output path, and apply the same reuse in the
corresponding wasi-data1 coverage module.
In `@services/ws-test-server/src/lib.rs`:
- Around line 108-137: Introduce a public type alias for the repeated WebSocket
stream type, then update the signatures of connect_agent and next_payload to use
the alias instead of the fully qualified type. Preserve all existing behavior
and visibility.
In `@utilities/int-gen/src/openapi.rs`:
- Around line 47-50: Update the reason text on the clippy::unwrap_used #[expect]
attribute to refer to “unwrap calls” instead of “expect calls,” leaving the lint
configuration and conversion behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ebcbb547-55e7-4ecd-b131-c760580da394
📥 Commits
Reviewing files that changed from the base of the PR and between 18dc4f9 and fb24dc8.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
Let the scripted server task terminate when the client disconnects.
Dropping the JoinHandle detaches a task that waits forever in pending(), retaining the WebSocket and task until the test runtime is torn down. Read from the socket until closure, or retain and abort the handle explicitly.
Proposed cleanup
-use futures_util::SinkExt as _;+use futures_util::{SinkExt as _, StreamExt as _};- std::future::pending::<()>().await;+ while ws.next().await.is_some() {}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/ws-test-server/tests/helpers.rs` around lines 19 - 33, Update the
scripted server task around the spawned WebSocket handler to stop waiting
indefinitely in std::future::pending. After sending the frames, read from ws
until the client closes the connection, allowing the task and socket to
terminate naturally; alternatively, retain the JoinHandle and explicitly abort
it during cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@services/ws-web-runner/build.rs`:
- Around line 17-22: Remove the crate-level clippy::unwrap_used expectation and
add narrowly scoped Windows-only #[expect] attributes to the link_mingw_shim and
run functions, preserving the existing justification. Leave unrelated
build-script code subject to the workspace lint.
---
Nitpick comments:
In `@services/ws-test-server/tests/helpers.rs`:
- Around line 19-33: Update the scripted server task around the spawned
WebSocket handler to stop waiting indefinitely in std::future::pending. After
sending the frames, read from ws until the client closes the connection,
allowing the task and socket to terminate naturally; alternatively, retain the
JoinHandle and explicitly abort it during cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1962d3c8-a20d-4778-a295-5ad8b8ee0c01
📥 Commits
Reviewing files that changed from the base of the PR and between fb24dc8 and 25bb50e.
📒 Files selected for processing (7)
config/ast-grep/rules/no-mod-in-tests.yaml
services/ws-test-server/src/lib.rs
services/ws-test-server/tests/helpers.rs
services/ws-test-server/tests/hub_forwarding.rs
services/ws-web-runner/Cargo.toml
services/ws-web-runner/build.rs
services/ws/src/lib.rs
✅ Files skipped from review due to trivial changes (1)
services/ws-web-runner/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (2)
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
The PR successfully introduces OpenTelemetry metrics and crucial stability fixes for the WebSocket hub, specifically addressing a panic risk in direct messaging. While Codacy indicates the code is up to standards, two high-severity issues must be addressed: a missing service.name attribute in the telemetry initialization which will cause metric assertions to fail, and an undeclared variable in the pydata1 module that will lead to runtime errors.
There is also significant scope creep beyond 'Add metrics', including a workspace-wide refactor of test error handling logic. While this improves consistency, it should be noted as it expands the surface area of the review. Additionally, the new coverage scripts introduce a dependency on goawk, which may impact developer environments.
About this PR
The coverage filter script in .mise/config.coverage.toml introduces a hard dependency on goawk, which may not be available in all developer or CI environments. Consider using a more common tool or documenting the requirement.
The PR scope significantly exceeds 'Add metrics'. It includes a workspace-wide refactor of test error handling (replacing .expect with .unwrap across many files) and updates to linting/link-check tasks. Please ensure these secondary changes are intentional and correctly implemented across all affected services.
2 comments outside of the diffservices/ws-modules/pydata1/pkg/et_ws_pydata1.js
line 21 🔴 HIGH RISK
Use of undeclared variable PYODIDE_CDN. Based on the file's constants, this appears to be a typo for PYODIDE_BASE_PATH.
line 11 🟡 MEDIUM RISK
The return value of the Promise executor is ignored. Avoid returning the result of resolve(). Use an explicit block for early exit to avoid confusion.
Test suggestions
Verify OTLP mock correctly decodes and flattens Sum, Gauge, and Histogram metrics from both JSON and Protobuf payloads.
Verify the hub increments the received message counter for every handled inbound WebSocket frame.
Verify the hub updates the active connection gauge correctly on client connect and disconnect.
Verify that sending a direct message to a non-existent agent returns an Invalid response and is logged without panicking.
Verify end-to-end integration where traces, logs, and metrics from the hub are all successfully captured by the mock collector.
Verify that next_payload correctly skips protocol-level frames like et-connect-ack and control frames like Pings.
The reason will be displayed to describe this comment to others. Learn more.
🔴 HIGH RISK
The service_label from the configuration should be added to service_descriptors as the service.name attribute. Without this, the mock collector's flatten_metrics and flatten_spans will fail to identify the service correctly, breaking tests in services/ws-test-server/tests/otel.rs.
The reason will be displayed to describe this comment to others. Learn more.
⚪ LOW RISK
Suggestion: Use the instrumentation library name (e.g. env!("CARGO_PKG_NAME")) instead of the service label as the tracer name. Service identity is already properly scoped via Resource attributes.
Align the expectation reason with the suppressed lint.
The “single call site” wording justifies clippy::single_call_fn, but Line 34 suppresses clippy::unwrap_used. Keep the reason focused on why these Windows build-script unwraps assert invariants.
As per coding guidelines: “Do not weaken or disable workspace Clippy lints without explicit operator permission; fix the code or use a narrowly scoped, justified #[expect].”
Proposed adjustment
- reason = "windows-gnu link setup: single call site, and unwraps assert build invariants"+ reason = "Windows linker setup unwraps assert build-script invariants"
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/ws-web-runner/build.rs` around lines 34 - 35, Update the reason
attached to the clippy::unwrap_used expectation in the Windows GNU link setup to
remove the “single call site” justification and focus solely on why these
unwraps validate build invariants. Keep the narrowly scoped lint expectation and
its existing suppression behavior unchanged.
Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@services/ws-web-runner/build.rs`:
- Around line 34-35: Update the reason attached to the clippy::unwrap_used
expectation in the Windows GNU link setup to remove the “single call site”
justification and focus solely on why these unwraps validate build invariants.
Keep the narrowly scoped lint expectation and its existing suppression behavior
unchanged.
ℹ️ Review info⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c93be5b5-6e7b-4e72-9ae7-c8e26ca9244a
📥 Commits
Reviewing files that changed from the base of the PR and between 25bb50e and 26a63ce.
📒 Files selected for processing (2)
services/ws-web-runner/build.rs
services/ws/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (1)
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
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
Summary by CodeRabbit