fix(otel): enrich http traces

[MasterPtato]

No description provided.

[MasterPtato]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• fix(pegboard): cache none runner pool errors too #3899
• fix(engine-runner): handle shutdown close codes correctly #3896
• chore(gas): add overview and history docs #3879
• fix(otel): enrich http traces #3878 👈 (View in Graphite)
• fix(guard): clean up guard #3862
• fix: add ready_chunks to worker bumps, bumps per tick metric #3855
• fix(epoxy): add metrics #3847
• chore: clean up errors #3846
• fix(pb): have restart policy actors sleep after pending for too long #3837
• fix: set failure reason regardless of crash policy #3835
• feat: add metrics to pb, guard #3815
• fix: clear all pending signals after workflow complete #3798
• fix: remove pending actors metric, fix actor error tracker, engine runner error print #3797
• chore(engine): add support for epoxy debugging #3794
• main

---

How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[claude]

PR Review: fix(otel): enrich http traces

Summary

This PR adds OpenTelemetry attributes to HTTP traces in both the API builder middleware and guard proxy service to improve observability. The changes add standard HTTP semantic conventions attributes to spans.

Code Quality ✅

Strengths:

• Clean implementation following OpenTelemetry semantic conventions
• Consistent pattern applied across both middleware and proxy service
• Properly uses the tracing-opentelemetry extension trait
• Adds the dependency correctly to workspace dependencies

Observations:

1. Redundant attribute setting in api-builder/middleware.rs (lines 70-71):
   The span is already created with method and uri fields (lines 65-66), and then immediately sets attributes for the same values (lines 70-71). While this provides both structured logging fields and OTEL attributes, it's redundant. The http.path attribute is set to the full URI string rather than just the path.

2. Inconsistent attribute naming:

   • Uses http.path but the value is actually the full URI (from req.uri().to_string())
   • Should either use url.full or http.target for the full URI, and url.path for just the path component
   • See OpenTelemetry HTTP semantic conventions

3. Missing standard HTTP attributes:
   While adding attributes is good, consider including other standard HTTP semantic convention attributes for better observability:

   • http.request.body.size
   • http.response.body.size
   • user_agent.original (already extracted in the code)
   • server.address and server.port
   • network.peer.address (the remote_addr already extracted)

Potential Issues ⚠️

1. String allocation on every request (performance):
   Each request creates multiple string allocations via .to_string() calls. For high-throughput services, consider if these allocations are necessary or if there's a more efficient way.

   • Line 70-71 in middleware.rs: 2 string allocations
   • Line 73: 1 string allocation
   • Line 2038-2039 in proxy_service.rs: 2 string allocations

   However, this is likely acceptable for the observability benefit, but worth noting.

2. MatchedPath extraction timing (api-builder/middleware.rs:72-74):
   The MatchedPath extension is checked before the request is processed by the router. Depending on when Axum populates this extension, it might not be available yet. Verify this works as expected - if the matched path isn't available at this point in the middleware chain, this code won't have any effect.

Security ✅

No security concerns identified. The changes only add observability attributes and don't modify request/response handling logic.

Test Coverage ⚠️

Missing tests:

• No new tests added to verify that the OTEL attributes are correctly set
• Existing test files in both packages don't appear to cover trace attribute verification

Recommendation:
Add integration tests that verify the expected attributes are present on spans. Example test structure:

• Create a test request
• Capture the span
• Assert expected attributes are set (http.request.method, http.path, http.response.status_code, etc.)

Performance Considerations ⚠️

Minor performance impact from:

1. String allocations on each request (discussed above)
2. Additional hash map insertions for span attributes

The overhead should be minimal and acceptable for the observability benefits, but should be monitored in production.

Suggestions for Improvement

1. Use OpenTelemetry HTTP semantic conventions consistently:

   // Instead of:
   req_span.set_attribute("http.path", req.uri().to_string());
   
   // Consider:
   req_span.set_attribute("url.full", req.uri().to_string());
   req_span.set_attribute("url.path", req.uri().path());

2. Add more standard attributes in a follow-up PR:

   • User agent, content lengths, network peer info, etc.

3. Add tests for trace enrichment:
   Verify spans contain expected attributes in integration tests.

4. Consider caching current_span in proxy_service.rs:
   You already extract it to a variable (line 2013), which is good for avoiding repeated Span::current() calls.

Verdict: ✅ Approve with Suggestions

This is a solid improvement to observability. The code is clean and follows good patterns. The main suggestions are:

• Use correct OpenTelemetry semantic convention attribute names
• Verify MatchedPath timing works as expected
• Consider adding tests for trace attributes
• Consider adding more standard HTTP attributes in follow-up work

The PR is ready to merge as-is, but the suggestions would make it even better.

---

Review generated by Claude Code

[pkg-pr-new]

More templates

• actor-actions
• example-ai-agent
• example-ai-and-user-generated-actors-freestyle
• chat-room
• example-cloudflare-workers
• example-cloudflare-workers-hono
• example-cloudflare-workers-inline-client
• cross-actor-actions
• example-cursors
• example-cursors-raw-websocket
• custom-serverless
• example-sqlite
• example-elysia
• example-experimental-durable-streams-ai-agent
• hello-world
• example-hono
• hono-react
• example-kitchen-sink
• multi-region
• native-websockets
• next-js
• example-raw-fetch-handler
• example-raw-websocket-handler
• example-raw-websocket-handler-proxy
• example-react
• scheduling
• state
• example-stream
• example-trpc

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3878

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3878

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3878

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3878

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3878

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3878

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3878

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@3878

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3878

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3878

commit: e8bbcf8