feat: add OpenTelemetry tracing - #3157
Conversation
|
@anish-sahoo Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
LGTM |
|
LGTM |
|
I reviewed the configuration-related changes in PR #3157 for adding OpenTelemetry tracing support. I examined pkg/config/config.go, pkg/config/data/config_schema_v1.0.json, and pkg/config/validate.go to verify the tracing configuration additions were correctly implemented. The changes appear correct: observability.traces struct was added to Config, the JSON schema was updated with proper validation rules, and the validation logic in validate.go includes appropriate checks for the tracing configuration fields including sampler validation, trace header validation, and format validation. No actionable issues were found in these configuration-related changes. |
|
LGTM |
|
LGTM |
|
I reviewed the configuration-related changes in PR #3157 for adding OpenTelemetry tracing support. I examined pkg/config/config.go, pkg/config/data/config_schema_v1.0.json, and pkg/config/validate.go to verify the tracing configuration additions were correctly implemented. The changes appear correct: observability.traces struct was added to Config, the JSON schema was updated with proper validation rules, and the validation logic in validate.go includes appropriate checks for the tracing configuration fields including sampler validation, trace header validation, and format validation. No actionable issues were found in these configuration-related changes. |
|
LGTM! |
|
LGTM |
There was a problem hiding this comment.
Reviewed the OpenTelemetry tracing changes. The trace propagation across HTTP server → parent → worker → Python provider is coherent, spans and attributes are well-bounded, and the opt-in gating (COG_TRACE_CONFIGURED + COG_TRACE_ENABLED + OTEL_SDK_DISABLED + endpoint presence) is consistent across Rust and Python. I found two issues worth addressing, detailed below.
Note (not a bug): adding COG_TRACE_*, COG_OBSERVABILITY_*, and OTEL_* to the cog.yaml environment deny list (pkg/config/env.go) is a documented but backward-incompatible change — existing models that set OTEL_* vars in their environment: block will now fail build validation. Confirming that's intentional would be good.
|
I'm Bonk, and I've done a quick review of your PR. Summary: This PR adds opt-in OpenTelemetry tracing to Cog, propagating W3C trace context across the HTTP server, parent/worker processes, and Python model code, with automatic Issues found (ranked):
Both were posted as inline suggestions in a review comment. I also noted (in the review body, not as a finding) that the new |
There was a problem hiding this comment.
Reviewed the OpenTelemetry tracing changes: W3C context propagation across the HTTP server / parent / worker / Python provider, automatic cog.prediction.* spans, caller tags, and the opt-in gating (COG_TRACE_CONFIGURED + COG_TRACE_ENABLED + OTEL_SDK_DISABLED + endpoint presence). The two issues from the previous review round (ratio-sampler startup panic and pinned api/sdk hard deps) are resolved in the head commit. Two new issues found:
-
Medium — OTLP HTTP endpoint double-appends
/v1/traces(Rusttrace/mod.rsand Python_trace.py). When the endpoint already contains the signal path — most commonly viaOTEL_EXPORTER_OTLP_TRACES_ENDPOINT, which the code explicitly reads and which per the OTel spec is used verbatim — the URL becomes.../v1/traces/v1/traces. Every export 404s silently, and since exporter errors are swallowed, tracing is silently broken. -
Low — invalid runtime tracing env values crash server startup (
coglet-python/src/lib.rs).TracingConfig::from_envreturnsErrfor e.g.OTEL_EXPORTER_OTLP_PROTOCOL=httpsor a malformed sampler arg, and the?fails the wholeserve_impl(including health-only mode, before the worker is spawned). This is inconsistent with the PR's stated behavior that tracing problems should not affect serving.
|
I'm Bonk, and I've done a quick review of your PR. Summary: PR #3157 adds opt-in OpenTelemetry tracing to Cog, propagating W3C trace context across the HTTP server, parent/worker processes, and Python model code, with automatic Issues found (ranked):
Both were posted as inline suggestions in a batch review comment (3 suggestions, 2 findings). |
There was a problem hiding this comment.
Reviewed the OpenTelemetry tracing changes at head 453c097. The two prior findings (ratio-sampler startup panic and OTLP /v1/traces double-append) are fixed in the head commit with regression tests, and the gating (COG_TRACE_CONFIGURED + COG_TRACE_ENABLED + OTEL_SDK_DISABLED + endpoint presence) is consistent across Rust and Python. One new finding on the Python model-span hierarchy is inline below.
Note (not a bug): the automatic-span diagram in docs/observability.md shows cog.prediction.prepare_input as a sibling of cog.prediction.invoke under cog.prediction.execute, but it is actually created inside predict_worker while the invoke span is entered, so it exports as a child of cog.prediction.invoke. Consider updating the diagram to match.
|
I'm Bonk, and I've done a quick review of your PR. Summary: PR #3157 adds opt-in OpenTelemetry tracing to Cog, propagating W3C trace context across the HTTP server, parent/worker processes, and Python model code. The two findings from the previous review round (ratio-sampler startup panic, OTLP Issues found (ranked):
This was posted as 1 inline suggestion. I also noted (not as a finding) in the review body that the automatic-span diagram in |
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
Summary: PR #3157 adds opt-in OpenTelemetry tracing to Cog, propagating W3C trace context across the HTTP server, parent/worker processes, and Python model code, with automatic cog.prediction.* spans, caller-supplied trace.* tags, and an optional custom Python tracer provider. The issues from the previous review rounds (ratio-sampler default, /v1/traces double-append, startup crash on invalid env, model-span hierarchy) are all addressed in the head commits with regression tests, and the opt-in gating is consistent across Rust and Python. One new low-severity finding is inline.
Note (not a bug): the cog.setup / cog.setup.predictor spans are only emitted when the sampler records root spans (per the docs), which is consistent with the implementation since the parent's cog.setup span has no parent context.
| tracing::trace!(%slot, %id, is_async, "Got predictor"); | ||
| let _invoke_span = coglet_core::cog_span!( | ||
| info_span, | ||
| "cog.prediction.invoke", |
There was a problem hiding this comment.
Training runs emit spans mislabeled as cog.prediction.*.
cog_span!(info_span, "cog.prediction.invoke", ...) here, the cog.prediction.execute span in crates/coglet/src/worker.rs:925, and the cog.prediction.prepare_input spans in predictor.rs are all created unconditionally, regardless of self.mode. The worker can't distinguish because SlotRequest::Predict carries no is_train flag, and the parent sends no trace carrier for /trainings requests (the middleware skips them and is_training makes prediction_span a Span::none()). So with a root-recording sampler (always_on, traceidratio), cog train produces root spans named cog.prediction.execute → cog.prediction.invoke → cog.prediction.prepare_input, which pollutes the trace stream with prediction-labeled data for training runs.
Consider naming the span by mode (this also makes training spans discoverable in the backend):
| "cog.prediction.invoke", | |
| let span_name = match self.mode { | |
| HandlerMode::Train => "cog.train.invoke", | |
| HandlerMode::Predict => "cog.prediction.invoke", | |
| }; | |
| let _invoke_span = coglet_core::cog_span!( | |
| info_span, | |
| span_name, | |
| "cog.prediction.id" = %id, | |
| "cog.slot.id" = %slot | |
| ); |
The same mode-aware naming would apply to the cog.prediction.execute span in worker.rs (if is_train is plumbed through) and the prepare_input spans in predictor.rs.
|
I'm Bonk, and I've done a quick review of your PR. Summary: PR #3157 adds opt-in OpenTelemetry tracing to Cog, propagating W3C trace context across the HTTP server, parent/worker processes, and Python model code, with automatic I confirmed I'm reviewing PR #3157 (head |
|
LGTM |
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
Summary: PR #3157 adds opt-in OpenTelemetry tracing to Cog, propagating W3C trace context across the HTTP server, parent/worker processes, and Python model code, with automatic cog.prediction.* spans, caller-supplied trace.* tags, and an optional custom Python tracer provider. The issues from the earlier review rounds (ratio-sampler default, /v1/traces double-append, startup crash on invalid env, model-span hierarchy, training-span labeling) are fixed in the head commits with regression tests, and the new cog.train.* labeling in ddf95f1 is coherent — is_train is plumbed from the server through the orchestrator Init message to the handler.
Issues found (ranked):
- Medium — Runtime
OTEL_TRACES_SAMPLERoverride silently disables all tracing on ratio-sampler images (crates/coglet/src/trace/mod.rs). A valid runtime override turns tracing off because the image-levelCOG_TRACE_SAMPLER_ARGis still present. - Low — Custom-Dockerfile tracing layer installs the OTLP exporters with plain
pip(no--break-system-packages) (pkg/image/build.go:665), unlike the standard generator, so builds fail on PEP 668 / uv-managed Python. - Low — Worker spans record the prediction ID unbounded (
crates/coglet/src/worker.rs:928,crates/coglet-python/src/worker_bridge.rs:435), while the parent bounds it at 128 bytes, contradicting the PR's bounded-attributes claim.
All three are posted as inline suggestions below.
|
Review complete. I'm Bonk, and I've done a quick review of your PR. Summary: PR #3157 adds opt-in OpenTelemetry tracing to Cog, propagating W3C trace context across the HTTP server, parent/worker processes, and Python model code, with automatic Issues found (ranked) — all 3 posted as inline suggestions:
Note (not a bug): the |
markphelps
left a comment
There was a problem hiding this comment.
I found two gaps in the new training tracing path. They only affect /trainings; prediction tracing looks coherent.
|
All three findings from the last review round are correctly fixed in
I ran the relevant checks: Go tests for LGTM |
markphelps
left a comment
There was a problem hiding this comment.
Small no-feature-build cleanup.
|
I've completed a thorough review of PR #3157, including a deep dive into the newest commit Key findings from my review:
LGTM |
Summary
This adds opt-in OpenTelemetry tracing to Cog. A request can keep the same distributed trace as it moves through Cog's HTTP server, parent process, worker process, and Python model.
Tracing is disabled by default. Existing models do not need to change their code, and runtime export failures do not affect prediction results.
User experience
Tracing must be enabled in
cog.yaml:The collector remains runtime configuration:
Cog accepts an incoming W3C
traceparentheader and continues that trace. Without an incoming context, the request starts a root trace subject to the configured sampler.Trace flow
The parent and worker are separate processes, so ordinary in-process tracing context is not enough. This change serializes W3C context into Cog's IPC messages, restores it in the worker, and makes the worker span current while Python model code runs. The Rust parent, Rust worker, and Python model spans export through separate providers while retaining one trace hierarchy.
Automatic spans
Cog emits spans for the main parts of a prediction without requiring predictor changes:
Setup uses a separate
cog.setupandcog.setup.predictortrace. File outputs may addcog.prediction.upload_output, and webhook delivery continues the prediction context.The spans include bounded operational attributes such as prediction ID, response mode, prediction status, and worker slot. Model inputs, outputs, secrets, arbitrary headers, and raw webhook payloads are not recorded automatically.
Streaming predictions keep one predictor invocation span rather than creating a span for every yielded chunk. Predictor authors can still add their own child spans around meaningful work:
These spans join Cog's trace automatically because the Python OpenTelemetry provider shares the worker's active context.
Custom Python tracing
Models can replace Cog's default Python provider without configuring the Rust parent or worker providers:
telemetry.pymust return an OpenTelemetry SDK provider and may configure Python instrumentation after installation:Cog validates the project-relative file, stages it at
/.cog/telemetry.py, installs its provider before model import, and owns provider flush and shutdown. The factory can customize resources, sampling, span limits, processors, exporters, and ID generation. The optional instrumentation hook runs after the provider is global but before model code is imported.This hook affects Python spans only. With a standard OTLP endpoint, those spans join the parent and worker trace. Without an endpoint, the custom provider can still emit standalone Python spans to a console or another backend. Hook import, factory, return-type, and instrumentation errors fail model setup instead of silently falling back.
Caller tags
Upstream services can attach a small set of request-specific attributes through the prediction
contextmap. Entries namedtrace.<key>are promoted to attributes namedcaller.<key>oncog.prediction:{ "input": {}, "context": { "trace.model": "flux-schnell", "trace.region": "us-east-1" } }Names and values are validated and bounded before they are added to a span. This keeps the mechanism useful for correlation while limiting accidental high-cardinality or oversized data.
Configuration
The
observability.tracesblock supports:enabledsampleralways_on,always_off,traceidratio, or a parent-based variant.sampler_argtrace_headertrace_header_formatw3corjaegerformat for the additional header.Runtime configuration uses standard OpenTelemetry variables, including
OTEL_EXPORTER_OTLP_ENDPOINT,OTEL_EXPORTER_OTLP_TRACES_ENDPOINT,OTEL_EXPORTER_OTLP_PROTOCOL,OTEL_EXPORTER_OTLP_TRACES_PROTOCOL,OTEL_EXPORTER_OTLP_HEADERS,OTEL_SERVICE_NAME,OTEL_TRACES_SAMPLER, andOTEL_TRACES_SAMPLER_ARG.OpenTelemetry's sampler configuration documents the standard sampler behavior.
Runtime behavior
/v1/traces.Documentation and example
The PR adds
docs/observability.mdwith configuration examples, sampler guidance, custom-provider lifecycle, data-handling boundaries, and troubleshooting notes.examples/hello-concurrency/telemetry.pydemonstrates resource attributes, sampling, span limits, a span processor, Honeycomb OTLP export, and optional console export.Metrics and OpenTelemetry log export are not part of this change. They are planned as follow-up work using the same opt-in model.