Native OpenTelemetry tracing — foundation + ADR (Phase 1a) - #689
Conversation
Groundwork for migrating Faust's built-in distributed tracing off the archived OpenTracing API to native OpenTelemetry, keeping the full span tree. This first increment adds the decision record and a self-contained, unit-tested tracing core; no existing call sites are rewired yet, so the running tracing layer is unaffected. - docs/adr/0001-native-opentelemetry-tracing.md: architecture decision record (options, why no opentelemetry-instrumentation-faust exists, semconv coexistence, re-expression of the two non-portable hacks, and the phased migration plan). - faust/utils/otel_tracing.py: api-only tracing primitives that degrade to no-ops when OpenTelemetry is absent -- tracer resolution against the global provider, contextvar current-span, deterministic rebalance parent context (native replacement for the immutable-context trace_id mutation), a deferred PendingSpan (native replacement for the finish monkeypatch), semconv-coexistence attribute helpers (legacy tags + messaging.*), and W3C header inject/extract. - tests/unit/utils/test_otel_tracing.py: 15 tests covering every primitive against an in-memory SDK exporter. - requirements/extras/opentelemetry.txt + setup.py: faust[opentelemetry] extra (opentelemetry-api/sdk). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HHPL4VFWQRQPpjR1gXSKyL
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #689 +/- ##
==========================================
+ Coverage 96.20% 96.24% +0.03%
==========================================
Files 110 111 +1
Lines 11790 11893 +103
Branches 1281 1294 +13
==========================================
+ Hits 11343 11446 +103
Misses 350 350
Partials 97 97 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The Phase 1a tests guard on `pytest.importorskip("opentelemetry.sdk.trace")`,
so without OpenTelemetry installed in CI they were all skipped, leaving
faust/utils/otel_tracing.py at 29% patch coverage and failing the codecov
gate.
- requirements/test.txt: pull in extras/opentelemetry.txt so the test job
installs opentelemetry-api/sdk and the 15 tests actually execute
(matches the existing `-r extras/*.txt` pattern in test.txt).
- otel_tracing.py: mark the dependency-absent `if not HAS_OTEL:` fallbacks
with `# pragma: no cover` -- they only run when OpenTelemetry is not
installed, a config CI no longer exercises.
- test_otel_tracing.py: add tests for the real runtime guards (None span,
None tracer, non-bytes key, default carrier) instead of pragma-ing them.
Module coverage is now 96%.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HHPL4VFWQRQPpjR1gXSKyL
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd9eedab7c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| carrier = {} | ||
| if not HAS_OTEL: # pragma: no cover | ||
| return carrier | ||
| propagate.inject(carrier) |
There was a problem hiding this comment.
When a producer span is made current through set_current_span(), as the existing tracing call sites do, this call still reads OpenTelemetry's separate global context rather than _current_span. Unless every caller also manually attaches an OTel context, outbound Kafka records will therefore receive no traceparent (or may receive a stale unrelated one), breaking producer-to-consumer trace continuity. Pass a context derived from current_span() to propagate.inject() or make the setter attach and restore the OTel context.
Useful? React with 👍 / 👎.
| self._span = self.tracer.start_span( | ||
| self.name, | ||
| context=self.context, | ||
| kind=kind, | ||
| attributes=self.attributes, |
There was a problem hiding this comment.
For a rebalance span queued while the Kafka generation is unknown, materialize() starts the span only when the generation later becomes available. The old lazy implementation started the span at the beginning of the operation and deferred only its finish, so this replacement omits all waiting time and reports a near-zero or otherwise shortened rebalance duration. Capture the intended start timestamp when PendingSpan is created and pass it as start_time when starting the real span.
Useful? React with 👍 / 👎.
| self.name = name | ||
| self.kind = kind | ||
| self.attributes = attributes | ||
| self.context = context |
There was a problem hiding this comment.
When a pending operation is created without an explicit context, this stores None, so start_span() resolves its parent from whatever OpenTelemetry context happens to be current later at materialization time. A rebalance can outlive the span or task that initiated it, causing the exported span to become a root or a child of an unrelated operation. Snapshot the current context when PendingSpan is constructed so deferred creation preserves the original trace hierarchy.
Useful? React with 👍 / 👎.
| digest = hashlib.blake2b(seed.encode(), digest_size=16).digest() | ||
| trace_id = int.from_bytes(digest, "big") |
There was a problem hiding this comment.
During a rolling migration where old OpenTracing members and native OTel members participate in the same Kafka generation, the old path derives the shared trace ID with Kafka's murmur2, while this function derives a different BLAKE2 value for the identical reb-{app_id}-{generation} seed. The generation's rebalance spans will therefore split into separate traces precisely while the compatibility path is in use, contrary to the ADR's stated murmur2-derived replacement. Retain the numeric murmur2 result when constructing the 128-bit OTel trace ID.
Useful? React with 👍 / 👎.
| span.set_attribute(_MSG_SYSTEM, "kafka") | ||
| span.set_attribute(_MSG_DESTINATION, topic) | ||
| span.set_attribute(_MSG_OPERATION_TYPE, operation) | ||
| if partition is not None: |
There was a problem hiding this comment.
For every span produced through these semantic-convention helpers, only messaging.operation.type is emitted, while the stable messaging conventions also require messaging.operation.name. Collectors and dashboards that classify messaging spans through the required operation-name attribute will therefore see these Faust spans as incomplete even though the module claims semantic-convention coexistence. Set messaging.operation.name alongside the type, using the concrete process or send operation supplied here.
Useful? React with 👍 / 👎.
Brings the OTel tracing foundation up to date with master, which has since landed the sensor-level OpenTelemetry path (#748), the OTel metrics monitor (#746), and moved `opentracing` out of core requirements into an extra (#686). Conflict resolutions: - `requirements/extras/opentelemetry.txt` (add/add): keep master's API floor and the fastapi/aiokafka instrumentation packages, and add `opentelemetry-sdk` on top so apps can configure an exporter out of the box. The tracing core itself still needs only the API. - `setup.py`: take master's bundle list, which already contains the `opentelemetry` entry this branch added. - `requirements/test.txt`: drop this branch's `-r extras/opentelemetry.txt` line; master already pulls the extra in, plus explicit api/sdk pins. The ADR is refreshed against the new tree: `opentracing` is described as an extra rather than a hard core dependency, a section explains how this work relates to `faust.contrib.opentelemetry` (sensor covers the message boundary, this covers the built-in span tree), and the phase list records what has already shipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TBE5RZ5mb1h5MjYyWGwmHZ
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22fee2433c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| """Extract a parent context from *carrier* (W3C trace headers).""" | ||
| if not HAS_OTEL: # pragma: no cover | ||
| return None | ||
| return propagate.extract(carrier) |
There was a problem hiding this comment.
When this helper receives Faust's normal Message.headers, propagation fails because Headers is commonly a list of (str, bytes) pairs (faust/types/core.py), while OpenTelemetry's default getter expects a mapping and does not decode Kafka's byte values. The existing OpenTelemetrySensor therefore supplies a custom getter that handles both representations; use the same normalization here, otherwise inbound Kafka records cannot continue their producer trace and list carriers may raise during extraction.
Useful? React with 👍 / 👎.
Master's project coverage has risen to 96.20%, which raises codecov's auto-target for this PR's patch above the 94.33% it has always measured, so `codecov/patch` and `codecov/project` went red on the sync commit rather than on any new code. Two causes, both in the new module: - `.coveragerc` excludes the `if typing.TYPE_CHECKING:` spelling, which is what the rest of faust uses. `otel_tracing.py` imports the name directly and writes bare `if TYPE_CHECKING:`, so the regex misses it and the two import lines under it count as unexecuted. Marked `# pragma: no cover`, matching the one other bare-form site (`faust/models/typing.py`). - The optional-argument arms of `add_consume_attributes` / `add_produce_attributes` were only ever called with every optional supplied, leaving four partial branches. Added two tests covering the topic-only path, which also pin the intended behavior: an unknown partition/key/offset is omitted from the span rather than recorded as a null. `faust/utils/otel_tracing.py` now reports 100% (103 statements, 26 branches). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TBE5RZ5mb1h5MjYyWGwmHZ
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ec48dcd7e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _MSG_PARTITION = "messaging.destination.partition.id" | ||
| _MSG_OPERATION_TYPE = "messaging.operation.type" | ||
| _MSG_KAFKA_KEY = "messaging.kafka.message.key" | ||
| _MSG_KAFKA_OFFSET = "messaging.kafka.offset" |
There was a problem hiding this comment.
When either messaging helper receives an offset, it records messaging.kafka.offset, which is not the Kafka semantic-convention key; the existing native OTel sensor correctly emits messaging.kafka.message.offset in faust/contrib/opentelemetry.py. Backends and dashboards querying the standard attribute will therefore lose offsets from all spans created through these new helpers. Rename this constant and update the corresponding tests.
Useful? React with 👍 / 👎.
| def cancel(self, note: str = "CANCELLED") -> None: | ||
| """Drop the pending span; if already materialized, mark and end it.""" | ||
| if self._span is not None and HAS_OTEL: |
There was a problem hiding this comment.
When cancel() is called before materialization—the documented path for dropping a pending operation—it records no canceled state, so a later or racing materialize() still starts and exports the span. This can occur when shutdown cancellation and generation discovery act on the same retained descriptor, defeating the promise that canceled pending work is dropped; latch the canceled state and make materialize() return None afterward.
Useful? React with 👍 / 👎.
2 participants
Description
Groundwork for migrating Faust's built-in distributed tracing off the archived / EOL OpenTracing API to native OpenTelemetry, while keeping the full processing-pipeline span tree (agents, streams, table recovery, assignor, timers, Crontabs, rebalance).
This is Phase 1a — foundation only. It adds an architecture decision record and a self-contained, unit-tested native tracing core. No existing call sites are rewired yet, so the running OpenTracing layer is completely unaffected. Opening as a draft to get agreement on the approach before the larger Phase 1b rewire.
Why native OTel (and why not an instrumentor)
opentelemetry-instrumentation-faustpackage — not on PyPI, not inopentelemetry-python-contrib, nothing community-maintained. The transport-level instrumentors (opentelemetry-instrumentation-aiokafka/-confluent-kafka, both beta) only patch the raw Kafka clients and cannot reproduce Faust's agent/stream/recovery/rebalance spans — they complement, they can't replace.opentelemetry-api(no SDK); calls are cheap no-ops until the app registers aTracerProvider.opentelemetry.contextis contextvars-based, so Faust's current-span propagation maps 1:1, including acrossawait.Relationship to
faust.contrib.opentelemetry(#748)Complementary, not competing.
OpenTelemetrySensorsits at the sensor layer — one{topic} processspan per event, which is what closes the aiokafka instrumentor's cross-thread gap — and it gives users native traces today without touchingapp.tracer. Sensors have no hooks for table recovery, the assignor, timers, Crontabs, the rebalance sequence, or the produce side; this work covers that built-in span tree, the ~134 call sites that today can only emit OpenTracing spans. After Phase 1b both emit into the same native OTel trace.What's in this PR
docs/adr/0001-native-opentelemetry-tracing.md— the decision record: options considered, how this relates to the sensor-level path, semantic-convention coexistence, native re-expression of the two non-portable OpenTracing hacks, and the phased migration plan.faust/utils/otel_tracing.py— api-only tracing primitives that degrade to no-ops when OpenTelemetry is absent:deterministic_parent_context()— native replacement for the oldspan.context.trace_id = murmur2(...)mutation (OTel span contexts are immutable): aNonRecordingSpanparent carries the seeded trace id and the real rebalance span inherits it;PendingSpan— native replacement for the_transform_span_lazyspan.finishmonkeypatch: defer span creation until the Kafka generation is known, or cancel;kafka-*tags (deprecated) and the OpenTelemetrymessaging.*attributes +SpanKind;traceparentheader inject/extract.tests/unit/utils/test_otel_tracing.py— 19 tests covering every primitive against an in-memory SDK exporter (both hacks validated end-to-end); the no-OTel degradation path is also exercised.requirements/extras/opentelemetry.txt— addsopentelemetry-sdkto the existing extra, so apps can configure an exporter out of the box. The tracing core itself needs onlyopentelemetry-api.Semantic-convention coexistence
Adopted additively: span names stay legacy by default (
consume-from-{topic},job-{topic},produce-to-{topic}) and are marked deprecated, whilemessaging.*attributes +SpanKindcoexist on the same span. A future opt-in flips names to semconv ({topic} process, …).Migration plan (see ADR)
opentracingmade optional (Make opentracing an optional dependency (faust[opentracing]) #686), shim bridge (Add optional faust[opentelemetry] extra + docs for OpenTelemetry via the OpenTracing shim #688), OTel metrics monitor (Add OpenTelemetry metrics monitor (faust[opentelemetry]) #746), sensor-level tracing (Pluggable web frameworks, OpenTelemetry tracing, and web feature flags #748) — all non-breaking.TracerTin OTel terms; ship a legacy adapter wrapping existing OpenTracingapp.tracerimplementations via the shim for one major cycle. Targets a major version —app.tracer/TracerTis a breaking signature change.faust[opentracing]extra along withfaust/utils/_opentracing.py.Notes for reviewers
🤖 Generated with Claude Code