Skip to content

feat(E8): add OTel context propagation to spawned child processes#256

Open
KooshaPari wants to merge 7 commits into
mainfrom
feat/E8-otel-context-propagate
Open

feat(E8): add OTel context propagation to spawned child processes#256
KooshaPari wants to merge 7 commits into
mainfrom
feat/E8-otel-context-propagate

Conversation

@KooshaPari

Copy link
Copy Markdown
Owner

Summary

Add OTel context propagation to child processes spawned by BytePort CLI and Go backend, enabling end-to-end distributed tracing across process boundaries.

Context

BytePort's OTel instrumentation (E6/E7/E9) added tracing and metrics within the Rust CLI process, but trace context was lost when spawning child processes (e.g., git ls-remote for repo validation). This created gaps in the trace tree, making it impossible to correlate parent-spawn operations.

Changes

  • Rust (byteport-otel/src/propagation.rs) — New module with:
    • current_context_envs() — injects W3C trace context into env pairs
    • propagate_to_cmd() / propagate_to_tokio_cmd() — convenience wrappers for std::process::Command and tokio::process::Command
    • EnvSetter trait for extensible env injection
    • Unit tests for no-op and active-span scenarios
  • Rust (init.rs) — Register global TraceContextPropagator in init_telemetry()
  • Go (backend/byteport/lib/otelpropagation.go) — New helper PropagateContextToCmd() for Go exec.Cmd
  • Go (backend/byteport/lib/apilink.go) — Wire propagation into ValidateGitRepo() (spawns git ls-remote)
  • Go (backend/byteport/main.go) — Register W3C propagation.TraceContext{} as global propagator

Use Cases

  • ValidateGitRepo: git ls-remote spawn now carries the parent HTTP request's trace context
  • Any future exec.Command calls in the Go backend inherit tracing
  • Rust CLI commands that spawn subprocesses (build, deploy, sidecar) are automatically linked

Testing

# Rust unit tests
cargo test -p byteport-otel propagation

# Go build
cd backend/byteport && go build ./...

# Manual verification — set OTEL_TRACES_EXPORTER=otlp and observe
# TRACEPARENT env var in spawned process

Links

  • DAG unit: E8 (compute/infra epic)
  • Prior work: E6 (byteport-otel crate), E7 (CLI tracing), E9 (CLI metrics)

@KooshaPari KooshaPari added the area:compute-infra Phenotype compute/infra epic label Jun 26, 2026
@gemini-code-assist

Copy link
Copy Markdown

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@github-actions

Copy link
Copy Markdown

Legacy Tooling Scan Report

Severity Count
Critical 0
High 0
Medium 0
Low 0

No violations detected.

This is a WARN-mode scan. Fix before strict enforcement begins.

@socket-security

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addedcargo/​futures@​0.3.3210010093100100
Addedcargo/​rayon@​1.10.010010093100100
Addedcargo/​tempfile@​3.27.09810093100100

View full report

@sonarqubecloud

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 92dfb0f662

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +152 to +153
Ok(TracerProvider::builder()
.with_batch_exporter(exporter, opentelemetry_sdk::runtime::Tokio)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Run OTLP initialization inside a Tokio runtime

When byteport-cli calls init_default() from its synchronous main (crates/byteport-cli/src/main.rs:108), this constructs the gRPC OTLP batch processor with the Tokio runtime even though no Tokio runtime has been entered. In normal CLI invocations that can panic during telemetry startup before any codec, transport, ui, or upload command runs; use a blocking/simple exporter for the CLI or initialize telemetry from an async Tokio runtime.

Useful? React with 👍 / 👎.

Comment on lines +108 to +109
let _otel_guard = byteport_otel::init::init_default();
info!("byteport-cli starting");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Send CLI telemetry logs somewhere other than stdout

byteport-cli uses stdout as its command output channel, but main now calls init_default() before parsing commands and the default config enables JSON fmt logging to stdout. With the new info! calls this means commands like codec encode hello no longer emit just 68656c6c6f; they are wrapped with log records, which breaks scripts/tests that consume the CLI output. Disable stdout logging by default for the CLI or write telemetry logs to stderr/a file.

Useful? React with 👍 / 👎.

Comment on lines +269 to +271
// Propagate OTel trace context to the child process so that
// git operations are linked to the parent trace.
PropagateContextToCmd(context.Background(), cmd)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Pass the active context into propagation

This always injects from context.Background(), which has no active span, so PropagateContextToCmd has no parent trace context to serialize and the spawned git ls-remote process will not be linked to the request/operation trace. In the path that validates a repo, thread the caller's request context into ValidateGitRepo and pass that context here instead of creating a fresh background context.

Useful? React with 👍 / 👎.

@kilo-code-bot

kilo-code-bot Bot commented Jun 27, 2026

Copy link
Copy Markdown

Code Review Summary

Status: 3 Issues Found | Recommendation: Address before merge

Overview

This PR adds OTel context propagation to child processes for end-to-end distributed tracing. The core concept is sound, but there are implementation issues that need fixing.

Severity Count
CRITICAL 1
WARNING 1
SUGGESTION 1
Issue Details (click to expand)

CRITICAL

File Line Issue
crates/byteport-otel/src/propagation.rs 44 inject() call missing context argument - OpenTelemetry 0.28 API requires &Context as first parameter

WARNING

File Line Issue
crates/byteport-otel/src/init.rs 57-61 set_text_map_propagator called inside if config.enable_tracing block - propagation won't work for metrics-only configuration

SUGGESTION

File Line Issue
backend/byteport/lib/otelpropagation.go 2-3 Function name PropagateContextToCmd uses PascalCase (Go convention is camelCase for function names)
Files Reviewed (7 files)
  • crates/byteport-otel/src/propagation.rs - CRITICAL: inject API misuse (lines 44, 139)
  • crates/byteport-otel/src/init.rs - WARNING: propagator registration scope
  • backend/byteport/lib/otelpropagation.go - SUGGESTION: naming convention
  • backend/byteport/lib/apilink.go - Reviewed (existing P2 issue on context.Background)
  • backend/byteport/main.go - Reviewed (correct W3C propagator registration)
  • crates/byteport-cli/src/main.rs - Reviewed (existing P2 issue on stdout logging)
  • crates/byteport-otel/src/tracing.rs - Reviewed (correct span helpers)

CRITICAL:
propagation.rs:44 - The inject method signature in opentelemetry 0.28 is propagator.inject(&cx, &mut carrier) where cx: &Context. The current code propagator.inject(&mut carrier) is missing the context argument. The context variable cx is defined on line 40 but never passed. This will cause propagation to fail silently or panic.

WARNING:
init.rs:57-61 - The propagator registration is inside the if config.enable_tracing block. If enable_tracing is false but enable_metrics is true, the propagator won't be registered and propagation to child processes will not work.

SUGGESTION:
otelpropagation.go:2-3 - Go convention uses camelCase for function names. PropagateContextToCmd should be propagateContextToCmd or propagateContext to match Go style.

EXISTING ISSUES (already commented):

  • init.rs:153 - OTLP batch exporter requires Tokio runtime in synchronous CLI main (P1)
  • main.rs:109 - stdout logging breaks CLI output contract (P2)
  • apilink.go:271 - context.Background() provides no active span for propagation (P2)

Fix these issues in Kilo Cloud


Reviewed by laguna-m.1-20260312:free · Input: 304.8K · Output: 18.2K · Cached: 2.8M

let mut carrier = Vec::new();

global::get_text_map_propagator(|propagator| {
propagator.inject(&mut carrier);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CRITICAL: Missing context argument in inject() call

The OpenTelemetry 0.28 TextMapPropagator::inject method signature is propagator.inject(cx, carrier). The current code calls propagator.inject(&mut carrier) without a context. The context variable cx is defined on line 40 but never passed to inject. This will cause propagation to fail silently or panic at runtime.

Suggested change
propagator.inject(&mut carrier);
global::get_text_map_propagator(|propagator| {
propagator.inject(&cx, &mut carrier);
});

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

let tracer = provider.tracer("test");

// We must register the global propagator.
opentelemetry::global::set_text_map_propagator(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CRITICAL: Same inject() call missing context in test code

Same issue as line 44 - propagator.inject(&mut carrier) is called without context. This test will not properly verify traceparent injection.

Reply with @kilocode-bot fix it to have Kilo Code address this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:compute-infra Phenotype compute/infra epic

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant