feat: add Sentry trace propagation from Rust to API #2147
Conversation
- Add TraceHeaders struct to owhisper-client for passing sentry-trace and baggage headers - Update ListenClientBuilder to support trace_headers() method - Inject trace headers into WebSocket upgrade requests - Add build_trace_headers() helper in listener plugin to extract Sentry context - Enrich Sentry scope with domain tags (session_id, stt_provider, channel_mode, model, languages, onboarding) - Add structured tracing logs with domain context at listener actor startup - Update apps/api/src/listen.ts to continue trace from headers using Sentry.continueTrace() - Add breadcrumbs at key WebSocket lifecycle points (connect, open, close, error) Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
✅ Deploy Preview for hyprnote-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for hyprnote ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughAdds end-to-end Sentry tracing: client-side trace headers, API handler tracing/span/breadcrumbs around WebSocket upgrade and upstream connection, and propagation of trace headers into the listener plugin which attaches them to upstream requests. Changes
Sequence DiagramsequenceDiagram
participant Client as Listen Client
participant Builder as ListenClientBuilder
participant API as API Handler
participant Upstream as Upstream Service
participant Listener as Listener Plugin
participant Sentry as Sentry
Client->>Builder: set TraceHeaders & build request
Builder->>Client: send request with sentry-trace / baggage (if set)
Client->>API: WebSocket upgrade request (with trace headers)
API->>Sentry: continueTrace(headers) / startSpan("listen")
API->>Sentry: add breadcrumb "start"
API->>Upstream: initiate upstream connection (guarded span)
Upstream-->>API: upstream connected
API->>Sentry: add breadcrumb "upstream_connected"
API->>Client: upgradeWebSocket -> onopen
API->>Sentry: add breadcrumb "client_opened"
Client->>API: send messages
API->>Upstream: forward messages
Upstream->>Listener: inbound request (includes sentry-trace/baggage)
Listener->>Sentry: build_trace_headers() (populate tags & extract headers)
Client->>API: close/error
API->>Sentry: add breadcrumb "client_closed"/"error"
API->>Sentry: finish span
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
plugins/listener/src/actors/listener.rs (1)
324-341: Consider extracting provider name without double adapter instantiation.The adapter is instantiated here solely to call
provider_name(), butbuild_single()will create another adapter instance internally. While this is likely cheap for most adapters, you could avoid the duplication by either:
- Having a static method on adapters for provider name, or
- Passing the adapter instance to the builder
This is a minor optimization that could be deferred.
apps/api/src/listen.ts (1)
77-79: Nitpick: Consider consistent use of nullish coalescing.Line 78 uses
??forcodebut line 79 uses||forreason. The difference is subtle (falsy vs nullish), but for consistency and to handle edge cases likecode: 0or empty string reasons, consider aligning the approach.onClose(event) { const code = event?.code ?? 1000; - const reason = event?.reason || "client_closed"; + const reason = event?.reason ?? "client_closed";owhisper/owhisper-client/src/lib.rs (1)
103-110: Consider adding a guard against empty trace headers.This code will add headers even if
sentry_traceorbaggagecontain empty strings. If the upstream system is strict about header format, empty values could cause issues. This relates to theunwrap_or_default()usage inlistener.rsthat could produce empty strings.if let Some(ref trace_headers) = self.trace_headers { - if let Some(ref sentry_trace) = trace_headers.sentry_trace { + if let Some(ref sentry_trace) = trace_headers.sentry_trace.as_ref().filter(|s| !s.is_empty()) { request = request.with_header("sentry-trace", sentry_trace.clone()); } - if let Some(ref baggage) = trace_headers.baggage { + if let Some(ref baggage) = trace_headers.baggage.as_ref().filter(|s| !s.is_empty()) { request = request.with_header("baggage", baggage.clone()); } }Alternatively, fix the source in
listener.rsto not produce empty strings (see related comment there).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
apps/api/src/listen.ts(1 hunks)owhisper/owhisper-client/src/lib.rs(3 hunks)plugins/listener/Cargo.toml(1 hunks)plugins/listener/src/actors/listener.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Agent implementations should use TypeScript and follow the established architectural patterns defined in the agent framework
Agent communication should use defined message protocols and interfaces
Files:
apps/api/src/listen.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/api/src/listen.ts
🧬 Code graph analysis (1)
plugins/listener/src/actors/listener.rs (2)
owhisper/owhisper-client/src/adapter/mod.rs (1)
provider_name(37-37)owhisper/owhisper-client/src/lib.rs (3)
trace_headers(64-67)default(37-45)adapter(69-77)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: Redirect rules - hyprnote-storybook
- GitHub Check: Header rules - hyprnote-storybook
- GitHub Check: Pages changed - hyprnote-storybook
- GitHub Check: Devin
- GitHub Check: fmt
- GitHub Check: desktop_ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: desktop_ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: desktop_ci (macos, depot-macos-14)
🔇 Additional comments (8)
plugins/listener/Cargo.toml (1)
66-67: LGTM!The workspace-level sentry dependency is appropriately added to support trace propagation from the listener plugin.
plugins/listener/src/actors/listener.rs (2)
327-333: LGTM!The structured logging with
session_id,channel_mode,model, andproviderfields provides excellent observability for debugging listener startup issues.
396-405: LGTM!Consistent logging pattern with the single path, maintaining good observability across both channel modes.
apps/api/src/listen.ts (3)
19-28: LGTM!Good use of
Sentry.continueTracecombined withstartSpanto properly propagate trace context from the Rust client through the API layer. The initial breadcrumb provides useful context for debugging.
41-54: Good error handling with appropriate HTTP status codes.The differentiation between timeout (504) and other upstream errors (502) is correct. The error is properly captured with Sentry and includes relevant tags for filtering.
110-114: No resource leak issue exists—closeConnections()properly cleans up the upstream WebSocket.The
closeConnections()method (apps/api/src/stt/connection.ts:145) comprehensively releases all resources when the handler fails:
- Checks upstream socket state before closing (line 168-169)
- Uses
safeCloseSocket()to safely close the upstream connection with error handling (line 171)- Clears all pending message queues and resets state variables (lines 174-183)
- Prevents double cleanup with the
shuttingDownflag guard (line 146-148)The implementation is robust and no further action is needed.
Likely an incorrect or invalid review comment.
owhisper/owhisper-client/src/lib.rs (2)
30-34: LGTM!Clean, minimal struct for trace header propagation. Deriving
CloneandDefaultis appropriate for this use case.
64-67: LGTM!Builder pattern method follows the established convention in this file.
- Changed sentry_trace extraction to use .filter(|s| !s.is_empty()) instead of .unwrap_or_default()
- Added same filter to baggage header for consistency
- This ensures None is returned when headers are absent or empty, rather than Some("")
Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
- Remove non-error breadcrumbs (onOpen, onClose, starting, upstream connected) to reduce noise - Use sentry::with_scope() instead of configure_scope() to prevent tags from leaking to unrelated events Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/listen.ts(1 hunks)plugins/listener/src/actors/listener.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Agent implementations should use TypeScript and follow the established architectural patterns defined in the agent framework
Agent communication should use defined message protocols and interfaces
Files:
apps/api/src/listen.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/api/src/listen.ts
🧬 Code graph analysis (1)
apps/api/src/listen.ts (3)
apps/api/src/stt/connection.ts (1)
WsProxyConnection(26-417)apps/api/src/stt/index.ts (3)
WsProxyConnection(6-6)createProxyFromRequest(17-46)normalizeWsData(7-7)apps/api/src/stt/utils.ts (1)
normalizeWsData(5-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - hyprnote-storybook
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote-storybook
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote-storybook
- GitHub Check: Pages changed - hyprnote
- GitHub Check: fmt
- GitHub Check: desktop_ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: desktop_ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: desktop_ci (macos, depot-macos-14)
- GitHub Check: Devin
🔇 Additional comments (13)
apps/api/src/listen.ts (5)
16-17: LGTM: Trace header extraction.The extraction of
sentry-traceandbaggageheaders for distributed tracing is correct and follows Sentry best practices.
19-22: LGTM: Trace continuation and span initialization.The use of
Sentry.continueTrace()to propagate the trace context andstartSpan()to create a child span is correct. The span name includes the provider, which aids in observability.
44-44: Note: Duration measurement includes upgrade time.The
connectionStartTimeis set after successful upstream preconnection but before the WebSocket upgrade completes. The duration metric (line 64) will therefore include the upgrade handshake time. This is reasonable for measuring the full lifecycle from upstream ready to client disconnect, but worth noting in case you intended to measure only the active WebSocket connection duration.
46-83: LGTM: Comprehensive WebSocket lifecycle handling.The WebSocket lifecycle callbacks are well-instrumented with Sentry breadcrumbs and exception capture. The error handling correctly captures both
Errorinstances and other event types, and theprovidertag aids in filtering and analysis.
85-90: LGTM: Upgrade failure handling.The upgrade failure path correctly cleans up the upstream connection before returning an error. (See separate comment about metrics imbalance.)
plugins/listener/src/actors/listener.rs (8)
9-9: LGTM: TraceHeaders import.The addition of
TraceHeadersto the import statement is necessary for the trace propagation functionality.
329-338: LGTM: Enhanced logging and trace header integration (single adapter).The adapter instantiation, trace header building, and enhanced structured logging with
session_idandprovidersignificantly improve observability for debugging.
345-345: LGTM: Trace header propagation to client builder.The trace headers are correctly passed to the
ListenClientbuilder via.trace_headers(trace_headers).
357-363: LGTM: Enhanced error logging with session context.Adding
session_idto error logs for timeouts and connection failures provides valuable context for debugging and correlating issues across services.
401-410: LGTM: Enhanced logging and trace header integration (dual adapter).The dual adapter path mirrors the single adapter changes, maintaining consistency across both code paths.
417-417: LGTM: Trace header propagation to client builder (dual path).Consistent with the single adapter path, trace headers are correctly propagated.
429-435: LGTM: Enhanced error logging with session context (dual path).Session context added to error logs maintains consistency with the single adapter path.
273-311: Verify: Tag propagation in trace headers.The function uses
sentry::with_scopeto set tags and thensentry::configure_scopeto extract headers from the span. While the empty string filtering (.filter(|s| !s.is_empty())) correctly addresses the past review concern, it's unclear whether the tags set viascope.set_tag()are automatically included in the extractedbaggageheader or if they're only for local observability in this service.Please confirm that the Sentry Rust SDK includes scope tags in the propagated baggage header when calling
span.iter_headers(), or if additional steps are needed to ensure these context tags (session_id, stt_provider, etc.) are propagated to the downstream API service.You can verify this by checking the Sentry Rust SDK documentation for the current version in use:
How does Sentry Rust SDK propagate scope tags in baggage headers when using span.iter_headers()?
| let connection: WsProxyConnection; | ||
| try { | ||
| connection = createProxyFromRequest(clientUrl, c.req.raw.headers); | ||
| await connection.preconnectUpstream(); | ||
| Metrics.websocketConnected(provider); | ||
| } catch (error) { | ||
| Sentry.addBreadcrumb({ | ||
| category: "websocket", | ||
| message: "Upstream connection failed", | ||
| level: "error", | ||
| data: { provider, error: String(error) }, | ||
| }); | ||
| Sentry.captureException(error, { | ||
| tags: { provider, operation: "upstream_connect" }, | ||
| }); | ||
| const detail = | ||
| error instanceof Error ? error.message : "upstream_connect_failed"; | ||
| const status = detail === "upstream_connect_timeout" ? 504 : 502; | ||
| return c.json({ error: "upstream_connect_failed", detail }, status); | ||
| } |
There was a problem hiding this comment.
Minor: Potential metrics imbalance on upgrade failure.
Metrics.websocketConnected is called after upstream preconnection succeeds (line 27), but if the WebSocket upgrade fails later (lines 86-88), Metrics.websocketDisconnected is never called because onClose won't fire. This creates an imbalance where a connection is counted but the corresponding disconnection isn't.
Consider tracking whether metrics were recorded and calling Metrics.websocketDisconnected in the upgrade failure path:
const response = await handler(c, next);
if (!response) {
connection.closeConnections();
+ Metrics.websocketDisconnected(provider, performance.now() - connectionStartTime);
return c.json({ error: "upgrade_failed" }, 400);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/api/src/listen.ts around lines 23 to 42, Metrics.websocketConnected is
incremented after preconnectUpstream but may never be decremented if the
subsequent WebSocket upgrade fails; add a local boolean (e.g.,
connectedMetricRecorded = false) set to true immediately after calling
Metrics.websocketConnected, then in the upgrade failure/error path (the code
handling the upgrade failure around lines 86–88) check that flag and call
Metrics.websocketDisconnected before returning/closing so every successful
websocketConnected call always has a corresponding websocketDisconnected call.
feat: add Sentry trace propagation from Rust to API
Summary
This PR implements distributed tracing between the Rust desktop app and the Bun API by propagating Sentry trace headers (
sentry-traceandbaggage) through WebSocket connections. Previously, traces from the Rust side and API side were disconnected, making it difficult to correlate issues across the full audio transcription pipeline.Changes:
TraceHeadersstruct toowhisper-clientwith builder support for injecting trace headers into WebSocket upgrade requestsbuild_trace_headers()helper in the listener plugin that extracts current Sentry span context and enriches the scope with domain tags (session_id, stt_provider, channel_mode, model, languages, onboarding)apps/api/src/listen.tsto continue traces from incoming headers usingSentry.continueTrace()and added breadcrumbs for error cases onlyUpdates since last revision
unwrap_or_default()to.filter(|s| !s.is_empty())so thatsentry_traceandbaggagebecomeNonewhen headers are absent or empty, rather thanSome("")sentry::configure_scope()tosentry::with_scope()in Rust to prevent tags from leaking to unrelated events across sessionsReview & Testing Checklist for Human
scope.get_span()returns a valid span: The trace header extraction depends on an existing Sentry span being present in the listener context. If no span exists, trace headers will be empty and propagation won't workwith_scopeusage in Rust: The current implementation sets tags in the scope config closure then reads headers in the inner closure - verify this correctly applies tags to the temporary scope before readingsentry-traceandbaggageheaders are actually received by the API (can add temporary logging or check network inspector)Recommended test plan:
Notes
span.iter_headers()API is used to extract trace headers - this should work but I couldn't verify against a live Sentry instancebuild_trace_headers()is called, the trace headers will be empty and the API will start a new trace instead of continuing oneLink to Devin run: https://app.devin.ai/sessions/1bde78b933a04425907664e6d351d758
Requested by: yujonglee (@yujonglee)