Conversation
propagation
- Add tower-http trace and request-id features to api/Cargo.toml
- Wire SetRequestIdLayer, PropagateRequestIdLayer, and TraceLayer in
app.rs
- Each request gets a UUID x-request-id (generated if not provided by
upstream)
- http_request span carries request_id, method, uri
- Response logs status and latency at info level
- Failures log error and latency at error level
- Add #[tracing::instrument] to all handlers in servers, channels, role,
server_members, server_invitations, and friend modules
- skip(state) on every handler to avoid logging service internals
- fields(user_id = ...) on every authenticated handler for Loki
correlation
Closes #130
📝 WalkthroughWalkthroughThis pull request implements HTTP request instrumentation and correlation ID propagation across the API. It adds tower-http dependencies for request ID generation and distributed tracing, wires middleware into the application router, and decorates handler functions with tracing instrumentation to capture user context and operation metadata. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Router as API Router
participant RequestID as RequestID Middleware
participant Trace as TraceLayer
participant Handler as HTTP Handler
participant Service as Service Layer
Client->>Router: HTTP Request
Router->>RequestID: Pass Request
RequestID->>RequestID: Generate x-request-id
RequestID->>Trace: Propagate x-request-id
Trace->>Trace: Create Span<br/>(request_id, method, uri)
Trace->>Handler: Invoke Handler
Handler->>Handler: Emit #[instrument]<br/>(add user_id field)
Handler->>Service: Process Business Logic
Service-->>Handler: Result
Handler-->>Trace: Return Response
Trace->>Trace: Log Response<br/>(status, latency)
Trace-->>Client: HTTP Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/http/channels/handlers.rs`:
- Line 68: The tracing span for create_private_channel omits user_id because
_user_identity is skipped and no fields(...) is declared; fix by not skipping
_user_identity (use skip(state) instead of skip(state, _user_identity)) and add
a fields(user_id = ...) entry that records the authenticated user's id from
_user_identity (e.g., user_id = _user_identity.user_id displayed), or if you
prefer the skip_all pattern keep skip_all but still declare fields(user_id =
...) that reads the id from the request identity; update the
#[tracing::instrument(...)] attribute on create_private_channel accordingly.
In `@api/src/http/server_invitations/handlers.rs`:
- Line 68: The span on get_invitation currently skips _user_identity and doesn't
set a user_id field, so add a fields entry referencing the request's user id
like in create_private_channel; update the attribute on the get_invitation
handler from #[tracing::instrument(skip(state, _user_identity))] to include
fields(user_id = %_user_identity.user_id) (or rename the parameter to
user_identity and use %user_identity.user_id) so the span contains the user_id
for correlation.
In `@api/src/http/servers/handlers.rs`:
- Line 34: The tracing::instrument attribute on the handler currently only skips
state but will record full Debug of user_identity, Json(request) and other
extractors; change the attribute to skip_all (e.g., replace
#[tracing::instrument(skip(state), fields(user_id = %user_identity.user_id))]
with a skip_all variant) and keep only the explicit fields(...) you want logged
(user_id) so UserIdentity, request bodies (Json(request)) and other extractors
are not serialized into spans; apply this same pattern to all handlers that use
tracing::instrument to avoid leaking sensitive or verbose data.
| (status = 500, description = "Internal server error") | ||
| ) | ||
| )] | ||
| #[tracing::instrument(skip(state, _user_identity))] |
There was a problem hiding this comment.
Missing user_id field on create_private_channel breaks Loki correlation for this endpoint.
This is the only authenticated handler in the PR that omits the user_id field. The _user_identity is skipped and no fields(user_id = ...) is declared, so this endpoint's spans will lack user context — inconsistent with every other instrumented handler.
🔧 Proposed fix
-#[tracing::instrument(skip(state, _user_identity))]
+#[tracing::instrument(skip(state), fields(user_id = %_user_identity.user_id))]Or if adopting skip_all as suggested elsewhere:
-#[tracing::instrument(skip(state, _user_identity))]
+#[tracing::instrument(skip_all, fields(user_id = %_user_identity.user_id))]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[tracing::instrument(skip(state, _user_identity))] | |
| #[tracing::instrument(skip(state), fields(user_id = %_user_identity.user_id))] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/src/http/channels/handlers.rs` at line 68, The tracing span for
create_private_channel omits user_id because _user_identity is skipped and no
fields(...) is declared; fix by not skipping _user_identity (use skip(state)
instead of skip(state, _user_identity)) and add a fields(user_id = ...) entry
that records the authenticated user's id from _user_identity (e.g., user_id =
_user_identity.user_id displayed), or if you prefer the skip_all pattern keep
skip_all but still declare fields(user_id = ...) that reads the id from the
request identity; update the #[tracing::instrument(...)] attribute on
create_private_channel accordingly.
| (status = 500, description = "Internal server error") | ||
| ) | ||
| )] | ||
| #[tracing::instrument(skip(state, _user_identity))] |
There was a problem hiding this comment.
Missing user_id field on get_invitation — same inconsistency as create_private_channel.
_user_identity is skipped and no fields(user_id = ...) is set, so this span lacks user correlation.
🔧 Proposed fix
-#[tracing::instrument(skip(state, _user_identity))]
+#[tracing::instrument(skip(state), fields(user_id = %_user_identity.user_id))]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/src/http/server_invitations/handlers.rs` at line 68, The span on
get_invitation currently skips _user_identity and doesn't set a user_id field,
so add a fields entry referencing the request's user id like in
create_private_channel; update the attribute on the get_invitation handler from
#[tracing::instrument(skip(state, _user_identity))] to include fields(user_id =
%_user_identity.user_id) (or rename the parameter to user_identity and use
%user_identity.user_id) so the span contains the user_id for correlation.
| (status = 500, description = "Internal server error") | ||
| ) | ||
| )] | ||
| #[tracing::instrument(skip(state), fields(user_id = %user_identity.user_id))] |
There was a problem hiding this comment.
Consider skipping additional params to avoid logging sensitive or verbose data.
#[tracing::instrument] records all non-skipped parameters via their Debug impl. Currently only state is skipped, so user_identity (which may contain auth context beyond just the user_id), Json(request) bodies, and other extractors will be fully serialized into every span. Since user_id is already captured explicitly via fields(...), the full UserIdentity is redundant and potentially leaks auth internals.
This applies uniformly across all handler files in this PR — suggest using skip_all and relying solely on fields(...) for the data you want:
♻️ Example fix (apply pattern to all handlers)
-#[tracing::instrument(skip(state), fields(user_id = %user_identity.user_id))]
+#[tracing::instrument(skip_all, fields(user_id = %user_identity.user_id))]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[tracing::instrument(skip(state), fields(user_id = %user_identity.user_id))] | |
| #[tracing::instrument(skip_all, fields(user_id = %user_identity.user_id))] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/src/http/servers/handlers.rs` at line 34, The tracing::instrument
attribute on the handler currently only skips state but will record full Debug
of user_identity, Json(request) and other extractors; change the attribute to
skip_all (e.g., replace #[tracing::instrument(skip(state), fields(user_id =
%user_identity.user_id))] with a skip_all variant) and keep only the explicit
fields(...) you want logged (user_id) so UserIdentity, request bodies
(Json(request)) and other extractors are not serialized into spans; apply this
same pattern to all handlers that use tracing::instrument to avoid leaking
sensitive or verbose data.
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive HTTP request instrumentation and correlation ID propagation to enable better observability and incident reconstruction in Loki. The changes implement request-level tracing using tower-http middleware and handler-level spans using the tracing crate's instrument macro.
Changes:
- Added tower-http middleware stack (SetRequestIdLayer, PropagateRequestIdLayer, TraceLayer) to generate and propagate x-request-id headers and create http_request spans with request_id, method, and uri fields
- Instrumented all HTTP handlers across servers, channels, roles, server_members, server_invitations, and friends modules with #[tracing::instrument] attributes that capture user_id for authenticated endpoints
- Updated dependencies to include tower 0.5 and enabled trace/request-id features on tower-http 0.6.6
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| api/Cargo.toml | Added tower dependency and enabled trace/request-id features for tower-http |
| Cargo.lock | Updated dependency resolution for tower and tower-http with new features |
| api/src/app.rs | Added middleware stack with SetRequestIdLayer, PropagateRequestIdLayer, and TraceLayer to generate UUID request IDs, propagate them to responses, and create http_request spans with logging for responses and failures |
| api/src/http/servers/handlers.rs | Added #[tracing::instrument] to all 7 handlers with skip(state) and user_id field extraction |
| api/src/http/channels/handlers.rs | Added #[tracing::instrument] to all 6 handlers with skip(state) and user_id field extraction (or skip _user_identity where appropriate) |
| api/src/http/role/handlers.rs | Added #[tracing::instrument] to all 9 handlers with skip(state) and user_id field extraction, including skipping request body in update_role |
| api/src/http/server_members/handlers.rs | Added #[tracing::instrument] to all 4 handlers with skip(state) and user_id field extraction |
| api/src/http/server_invitations/handlers.rs | Added #[tracing::instrument] to all 3 handlers with skip(state) and user_id field extraction (or skip _user_identity where appropriate) |
| api/src/http/friend/handlers.rs | Added #[tracing::instrument] to all 8 handlers with skip(state) and user_id field extraction |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| (status = 500, description = "Internal server error") | ||
| ) | ||
| )] | ||
| #[tracing::instrument(skip(state, request), fields(user_id = %user_identity.user_id))] |
There was a problem hiding this comment.
The instrumentation here skips the request parameter to avoid logging potentially large or sensitive JSON payloads. However, this pattern is not consistently applied across all handlers that accept JSON bodies. For consistency and to prevent logging sensitive data, all handlers that accept JSON request bodies should skip them in their instrumentation.
Affected handlers that should also skip their JSON request parameters:
create_serverin servers/handlers.rs (line 38)update_serverin servers/handlers.rs (line 159)create_server_channelin channels/handlers.rs (line 41)create_private_channelin channels/handlers.rs (line 72)update_channelin channels/handlers.rs (line 160)create_rolein role/handlers.rs (line 48)create_invitationin server_invitations/handlers.rs (line 42)update_memberin server_members/handlers.rs (line 142)create_friend_requestin friend/handlers.rs (line 173)accept_friend_requestin friend/handlers.rs (line 200)decline_friend_requestin friend/handlers.rs (line 226)
For example, these should use patterns like:
#[tracing::instrument(skip(state, request), fields(user_id = %user_identity.user_id))]#[tracing::instrument(skip(state, input), fields(user_id = %user_identity.user_id))]
propagation
Closes #130
Summary by CodeRabbit