Skip to content

chore(envoy-client): add envoy key tracing context#5002

Merged
NathanFlurry merged 1 commit into
mainfrom
chore/envoy-client-envoy-key-logging
May 9, 2026
Merged

chore(envoy-client): add envoy key tracing context#5002
NathanFlurry merged 1 commit into
mainfrom
chore/envoy-client-envoy-key-logging

Conversation

@NathanFlurry
Copy link
Copy Markdown
Member

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Copy link
Copy Markdown
Member Author

NathanFlurry commented May 9, 2026

@railway-app
Copy link
Copy Markdown

railway-app Bot commented May 9, 2026

🚅 Deployed to the rivet-pr-5002 environment in rivet-frontend

Service Status Web Updated (UTC)
kitchen-sink ❌ Build Failed (View Logs) Web May 9, 2026 at 7:33 am
website 😴 Sleeping (View Logs) Web May 9, 2026 at 2:54 am
frontend-inspector ❌ Build Failed (View Logs) Web May 9, 2026 at 2:44 am
mcp-hub ✅ Success (View Logs) Web May 9, 2026 at 2:43 am
ladle ❌ Build Failed (View Logs) Web May 9, 2026 at 2:43 am
frontend-cloud ❌ Build Failed (View Logs) Web May 9, 2026 at 2:42 am

@railway-app railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5002 May 9, 2026 02:41 Destroyed
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 9, 2026

PR Review: chore(envoy-client): add envoy key tracing context

Overview

This PR adds envoy_key as a structured tracing field across envoy-client spans and named spans for long-lived async tasks (envoy_connection, envoy_ws_write, envoy_client, envoy_graceful_shutdown). It is a pure observability improvement with no functional changes.

What Works Well

  • Follows project logging conventions: envoy_key = %shared.envoy_key uses structured fields rather than string interpolation in the log message, matching CLAUDE.md.
  • Correct .instrument() usage: all futures are instrumented before being handed to tokio::spawn / spawn_local / spawn_detached, so the span context travels with the task correctly.
  • Explicit parent capture on detached task: in envoy.rs handle_shutdown, parent: tracing::Span::current() is correctly used so the detached spawn_detached task does not lose its parent span context.
  • Log messages remain lowercase, consistent with project conventions.
  • Both native.rs and wasm.rs receive the same tracing additions (wasm parity).

Suggestions / Minor Issues

1. Span level mismatch for the root envoy span

envoy_client is created as an info_span! while the child spans (envoy_connection, envoy_ws_write) are debug_span!. This means the root span is always recorded but its children are only recorded at debug level. Consider making all long-lived connection spans the same level (probably debug) unless you specifically want envoy_client to be always-on in production traces.

2. Redundant envoy_key clone in wasm.rs onmessage closure

let envoy_key = shared.envoy_key.clone();
// ...
tracing::warn!(%envoy_key, "received non-binary websocket message");

This closure runs inside the envoy_connection span (via .instrument(span)), so envoy_key is already present on the ambient span. The explicit field on the log line is not harmful, but the clone is unnecessary if you rely on the ambient span field instead.

3. Incomplete coverage in handle.rs

Only start_serverless_actor gets envoy_key added. Other methods on EnvoyHandle that emit logs do not. Fine as an incremental improvement, but a follow-up pass to add it to remaining callsites would complete the picture.

4. shared2 used before move for span creation in native.rs (non-issue, just a note)

shared2 was already being cloned for the async move block; reading envoy_key from it before the move for the span is correct. No change needed.

Summary

Clean, focused change with no correctness concerns. The span-level mismatch between info_span! (root) and debug_span! (children) is the only nit worth addressing before merge. LGTM otherwise.

@NathanFlurry NathanFlurry force-pushed the 05-06-chore_rivetkit_rewrite_work_registry_fix_waituntil_not_preventing_sleep branch from 6a62381 to 6b5aa7b Compare May 9, 2026 07:33
@NathanFlurry NathanFlurry force-pushed the chore/envoy-client-envoy-key-logging branch from 283da30 to 5fddabc Compare May 9, 2026 07:33
@railway-app railway-app Bot temporarily deployed to rivet-frontend / rivet-pr-5002 May 9, 2026 07:33 Destroyed
Base automatically changed from 05-06-chore_rivetkit_rewrite_work_registry_fix_waituntil_not_preventing_sleep to main May 9, 2026 07:50
@NathanFlurry NathanFlurry merged commit 5fddabc into main May 9, 2026
11 of 18 checks passed
@NathanFlurry NathanFlurry deleted the chore/envoy-client-envoy-key-logging branch May 9, 2026 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant