APP-14871: Report WebRTC client connection metadata - #189
Draft
Daniel Botros (danielbotros) wants to merge 3 commits into
Draft
Daniel Botros (danielbotros) wants to merge 3 commits into
Daniel Botros (danielbotros) wants to merge 3 commits into
Conversation
Port of goutils#583. After a WebRTC dial finishes (success or failure), best-effort report to the app signaling server it dialed through: the furthest dial stage reached, the gRPC failure code, the dial duration, how the dial was signaled, and the selected ICE candidate pair per side (host/stun/relay + relay address). New src/rpc/dial_report.rs holds the reporting subsystem (stage tracker, candidate/signaling-path classification from peer-connection stats, report selection, and delivery). maybe_connect_via_webrtc is split into a reporting wrapper and an _inner that advances a StageTracker through the dial; the report is delivered on a detached task over the same already-authenticated, rpc-host-stamped signaling channel, so it never adds latency to the dial. The proto types are already vendored on main (Automated Protos Update #187), so no src/gen changes are needed. Reports are delivered only for cloud-signaled dials (app.viam.com / app.viam.dev), since only app implements ReportConnectionMetadata. Unlike Go, rust-utils does not signal WebRTC over mDNS and does not reconstruct a prod-app connection for raw-IP/.local dials, so local-signaled dials produce no report. The public C ABI is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Port the local-dial half of goutils#583's fixUpReportDialOpts. A WebRTC dial signaled through a robot's own (non-cloud) signaling server — dialing a .local.viam.cloud name or a LAN address directly — now reports too: the background task reconstructs an authenticated connection to prod app.viam.com, reusing the dial's credentials and stamping the robot's rpc-host, and delivers the report there (only app implements ReportConnectionMetadata; a robot's local signaling server does not aggregate these metrics). A locally-signaled dial with no credentials (WithoutCredentials) is dropped, matching Go. mDNS-discovered dials are direct gRPC, not WebRTC, so they have no report. Like Go, the app address is hard-coded to prod, so a staging robot dialed locally does not route to app.viam.dev. Delivery is threaded through a new ReportTarget enum: Cloud reuses the dial's channel, Local reconstructs the app connection. Reconstruction is bounded by a 5s timeout and stays entirely on the detached report task, so it never affects the dial. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address three differences from the Go implementation: - Suppress local-signaled WebRTC *failure* reports. rust-utils falls back to a direct gRPC connection on any WebRTC failure, and for a locally-signaled dial that direct connection to the robot generally succeeds — so the WebRTC failure did not fail the dial. should_deliver now reports a failure only for cloud-signaled dials (where direct gRPC can't reach the robot, so the failure is terminal), mirroring Go's suppression of non-READY reports on a successful dial. Successful (READY) dials still report on both paths. - Advance SIGNALING_CONNECTED at the start of the WebRTC dial rather than after the config RPC. The channel is already eagerly connected by create_channel, so a config-RPC failure against a reachable signaler now reports SIGNALING_CONNECTED instead of UNSPECIFIED, matching Go. - Gate reporting off in test/CI harnesses via a VIAM_DISABLE_DIAL_REPORTING env var (cfg(test) only covers the crate's own unit tests, not integration tests or downstream consumers). The echo integration tests set it so no detached report task can outlive a test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.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
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.
Port of goutils#583 to rust-utils. After a WebRTC dial finishes — success or failure — the client best-effort reports to the app signaling server it dialed through: the furthest dial stage reached, the gRPC failure code, the dial duration, how the dial was signaled, and the selected ICE candidate pair per side (host / stun / relay, plus the relay address).
Changes
src/rpc/dial_report.rs— the reporting subsystem:StageTracker(monotonic atomic dial stage),classify_signaling_path,classify_connection/classify_candidate(readspeer.get_stats()and maps the nominated + succeeded ICE candidate pair to host/stun/relay + relay address),failure_code(extracts atonic::Statuscode, elseUNKNOWN),should_deliver(report-selection semantics), andsend_dial_report(best-effort delivery, 5s timeout). Includes unit tests.src/rpc/dial.rs—maybe_connect_via_webrtcis split into a reporting wrapper and an_inner; stageadvance()calls are threaded through the dial, the spawned call-response task, and the ICE callback. Bothconnect_innervariants classify the signaling path and pass it down. The report is built and delivered on a detachedtokio::spawnover the same already-authenticated, rpc-host-stamped signaling channel, so reporting adds no latency to the dial.src/rpc/webrtc.rs—new_peer_connection_for_clienttakes the stage tracker and advancesDTLS_CONNECTEDon peer-connectionConnected.The proto types are already vendored on
main(Automated Protos Update #187), so nosrc/genchanges are needed.This PR is three commits: (1) cloud-signaled reporting; (2) the local-signaled case (Go's
fixUpReportDialOpts); (3) refinements to failure suppression, stage tracking, and test gating.Design notes / divergences from Go
app.viam.com/app.viam.dev) report over their own authenticated channel..local.viam.cloudname or a LAN address directly, i.e. WebRTC signaled through the robot's own signaling server — report to prod app, reconstructing an authenticated connection reusing the dial's credentials and stamping the robot's rpc-host (only app implementsReportConnectionMetadata; a robot's local signaling server doesn't aggregate these metrics). A locally-signaled dial with no credentials is dropped, matching Go. Like Go, the app address is hard-coded to prod, so a staging robot dialed locally isn't routed toapp.viam.dev.should_deliver: on success only aREADYreport is delivered. Failures are reported only for cloud-signaled dials — rust-utils falls back to direct gRPC on any WebRTC failure, and for a local dial that fallback to the robot generally succeeds, so the WebRTC failure did not fail the dial and is suppressed (a cloud robot isn't directly reachable, so a cloud WebRTC failure is terminal and is reported). This mirrors Go suppressing non-READY reports when the logical dial nonetheless succeeded.SIGNALING_CONNECTEDis set at the start of the WebRTC dial (the channel is already eagerly connected bycreate_channel), so a config-RPC failure against a reachable signaler reportsSIGNALING_CONNECTEDrather thanUNSPECIFIED, matching Go.cfg(test)and wheneverVIAM_DISABLE_DIAL_REPORTINGis set — the latter so integration-test / CI harnesses (whichcfg(test)doesn't cover) can opt out; the echo integration tests set it.viam_rust_utils.his unchanged (only internal functions changed).Downstream SDKs
No code changes are required in the C++ or Python SDKs. Both dial through the rust-utils FFI (
dial→DialBuilder::connect(), the instrumented path) and downloadlibviam_rust_utilsfromrust-utils/releases/latestat build time, so they pick this up automatically once it merges, a new rust-utils release is cut, and each SDK is rebuilt. Receiver-side requirement: the app signaling server must implementReportConnectionMetadata(otherwise the client getsUnimplemented, logged at debug — harmless).Testing
cargo buildclean;cargo clippyclean on the new/changed code.cargo test --lib— all pass (4 newdial_reporttests).🤖 Generated with Claude Code