docs(rfc): specify RFC 0039 (inbound trace propagation) + 0040 (DataFusion operators) - #626
Merged
Merged
Conversation
…usion operators) Two traces-completeness RFCs, at `specified` after maintainer design sign-off (§5 criteria written, scenarios numbered). RFC 0039 — inbound trace-context propagation: a global TraceContextPropagator + per-ingress extract/set_parent so the ingest/query/MCP SERVER spans continue the caller's trace instead of starting roots. Notable: the ingest span is born after a tokio::spawn, so the extracted Context is carried across (the RFC0038.3 boundary, for the parent this time); and the trace OTel crates promote from dev- to prod-dependencies in ingester/server. RFC 0040 — DataFusion operator instrumentation: per-ExecutionPlan-node child spans under POST /v1/query, reconstructed post-hoc from the finished plan. DataFusion 54's BaselineMetrics records real wall-clock Start/EndTimestamp per operator, so the spans carry genuine bounds (not synthetic). A new datafusion+opentelemetry-only crate (ourios-df-otel) designed to lift to datafusion-contrib as datafusion-opentelemetry. Honours RFC0038.2 (O(plan), never per-record); zero cost when unsampled. Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds RFC 0039 on inbound W3C trace-context propagation and RFC 0040 on DataFusion operator instrumentation, including proposed designs, acceptance criteria, testing strategies, open questions, and references. Both RFCs are linked from ChangesObservability RFCs
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Ourios RFC documentation to move two traces-completeness designs (RFC 0039 and RFC 0040) to specified status, and links them into the mdBook navigation via docs/SUMMARY.md. This fits the repo’s RFC-driven development workflow by documenting the approved designs and their acceptance criteria before any implementation slices land.
Changes:
- Add RFC 0039 specifying inbound W3C trace-context propagation so existing SERVER spans continue caller traces.
- Add RFC 0040 specifying DataFusion physical-plan operator span reconstruction to deepen
POST /v1/queryinto an operator tree. - Register both RFCs in the mdBook table of contents.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| docs/SUMMARY.md | Adds RFC 0039 and RFC 0040 to the mdBook navigation. |
| docs/rfcs/0039-inbound-trace-context-propagation.md | New specified RFC describing inbound trace-context extraction and span parenting across ingress paths. |
| docs/rfcs/0040-datafusion-operator-instrumentation.md | New specified RFC describing post-hoc operator span emission from DataFusion ExecutionPlan metrics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/rfcs/0039-inbound-trace-context-propagation.md`:
- Around line 64-72: Correct the “Five span sites” statement to explicitly
define the scope as either four logical ingress categories or six concrete
span-producing functions, including the three MCP _traced functions in row D.
Ensure the surrounding table and implementation/test coverage use the same
counting convention without omitting any listed site.
- Around line 86-98: The RFC presents conflicting contracts for ingest context
hand-off. Choose either the explicit parent parameter approach or request
extensions, then align sections 3.3, 3.4, and 7, the implementation plan,
acceptance tests, and spawn-boundary test; if retaining request extensions,
explicitly test context preservation when the request moves into the spawned
task.
- Around line 74-75: Update the RFC guidance for cases C and D, including the
corresponding sections around lines 92–93 and 151–156, to require constructing
the span unentered, calling set_parent before entering it, and only then
activating the span. Document AlreadyStarted and LayerNotFound as distinct
set_parent outcomes rather than treating them as generic traces-disabled
failures.
In `@docs/rfcs/0040-datafusion-operator-instrumentation.md`:
- Around line 117-123: Update the metric attribute contract in the
instrumentation RFC around the MetricValue-derived attributes to normatively
specify each attribute’s type and unit, including the exact representation for
elapsed_compute, and define the row_groups_pruned/row_groups_matched behavior
when matched is zero. Ensure the span contract uses deterministic types, units,
and no-match values while preserving the existing OTel naming and
registry-validation requirements.
- Around line 151-159: Update the sampling-check description in the “3.6 Cost
discipline” section to use the parent span context via
parent_cx.span().span_context().is_sampled() rather than Context::is_sampled().
Ensure RFC0040.6 remains a unit test verifying the reconstruction walk is
skipped when the parent context is unsampled.
- Around line 89-93: The RFC’s OTel span example must match the 0.32 SDK API:
update record_plan_spans at
docs/rfcs/0040-datafusion-operator-instrumentation.md:89-93 to use a generic or
concrete tracer instead of &dyn Tracer, and at
docs/rfcs/0040-datafusion-operator-instrumentation.md:107-114 bind the created
span mutably before calling end_with_timestamp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c5c1f6a6-9a50-4cc3-b12f-fa89987a8d34
📒 Files selected for processing (3)
docs/SUMMARY.mddocs/rfcs/0039-inbound-trace-context-propagation.mddocs/rfcs/0040-datafusion-operator-instrumentation.md
…tor attrs RFC 0039: the biggest correction — set_parent returns AlreadyStarted on an entered #[instrument] span, so the parent would be silently dropped. Switch the whole mechanism to attaching the extracted context as current around the span future (opentelemetry::trace::FutureExt::with_context), so the root span inherits it. This also unifies the ingest carry-channel (one contract: extract + with_context across the spawn, no ingest_bound signature change) and shrinks the dep change to a single `trace` feature flag (tracing-opentelemetry no longer needed in the ingress crates). Ingress count corrected to four categories / six functions. RFC 0040: correct the span-emission example to the 0.32 API — generic `T: Tracer` (Tracer isn't object-safe: associated Span type), SystemTime timestamps via `SystemTime::from(DateTime<Utc>)`, `end_with_timestamp(&mut self)`. Make operator attributes normative (types + units; elapsed_compute in ns) and emit pruning as two counts (row_groups_pruned/matched) rather than a ratio undefined at matched==0. Specify the sampled gate as `parent.span().span_context().is_sampled()`. Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
Consulted the OpenTelemetry semantic conventions (the project's
consult-before-any-signal-naming rule) rather than leaving the names to
implementation time.
Findings: OTel defines NO convention for query-plan / per-operator spans —
the whole db.* span convention describes database *client* spans (one
app->DB operation). Reusing db.response.returned_rows for a plan node's
output rows would collide with its normative meaning ("rows returned by
the database operation ... at the time the span ends"), so the operator
attributes take a distinct namespace.
Namespace is `datafusion.operator.*`, not `ourios.*`: the semantics are
DataFusion's and the crate is built to be extracted (an ourios-prefixed
attribute would be wrong the moment another project uses it). The five
names are fixed in §3.3 and still go through semconv/registry/ + weaver so
live-check validates them.
Also records the naming for the deferred "show the query" question:
db.query.text (stable, carries a normative sanitization requirement) and
db.query.summary (stable, explicitly a low-cardinality grouping key) —
noting both are defined on client spans while ours is SERVER, and that the
DSL's user literals tie it to the §3.5 PII decision. Stays out of this RFC.
Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
jensholdgaard
added a commit
that referenced
this pull request
Jul 25, 2026
… slice 1) (#627) * feat(server): continue the caller's trace on the query span (RFC 0039 slice 1) First slice of RFC 0039: the W3C propagator, the extractor, and the query ingress. `POST /v1/query` now joins the caller's trace instead of rooting a new one. - ourios-telemetry `init` installs `TraceContextPropagator` unconditionally (§3.1): stateless, and extraction is inert with no traces pipeline, so ingress code needs no "is tracing on?" branch. - New `receiver::propagation` with one `HeaderExtractor` over `http::HeaderMap` plus `extract_context`, which resolves through the global propagator. One shim serves every ingress because the gRPC path will extract from raw HTTP headers at its tower layer (§3.4). - `handle_query` becomes an un-instrumented wrapper that extracts the context and runs the instrumented `handle_query_traced` under it via `FutureExt::with_context`. This is the §3.3 mechanism: `set_parent` fails with `AlreadyStarted` on an entered span, and a `#[tracing::instrument]` span is entered for its whole body, so the parent would be silently dropped. - Deps: `trace` added to the existing `opentelemetry` production dependency in ingester + server (§3.5). The API half only — the SDK propagator install stays in ourios-telemetry, and `tracing-opentelemetry` is not needed in the ingress crates because the attach idiom uses the layer's own current-context bridge. Tests: RFC0039.1 (query arm — span joins the caller's trace, parented to the caller's span), RFC0039.2 (no traceparent is a fresh valid root), RFC0039.5 (malformed traceparent is treated as absent, request still succeeds), plus extractor unit tests and a guard asserting the injected carrier really names the expected remote span. Remaining slices: the ingest spawn-boundary hand-off (RFC0039.3), MCP (RFC0039.6), and the sampling assertion (RFC0039.4). Refs #626. Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org> * test(server): install the test propagator once, not per call The RFC 0039 tests wrote the process-global propagator on every `query_spans` call. Tests in one binary run in parallel, so that is a concurrent write to shared global state — a real flakiness vector, and the "idempotent-safe" claim in the comment rested on unverified upstream behaviour rather than on anything guaranteed (Copilot, #627). Install it exactly once behind `std::sync::Once`, so every test observes a single install and nothing races. Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org> * docs(test): correct the propagator note in the RFC 0039 test module The module doc claimed the propagator is "passed explicitly rather than relying on the global one", but the integration tests deliberately install and use the process-global propagator — `extract_context`, the code under test, resolves through `global::get_text_map_propagator`, so it has to be global. Say that, and name the one test that really does construct a propagator directly (the carrier-meaning guard) (Copilot, #627). Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org> --------- Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
This was referenced Jul 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two traces-completeness RFCs at
specifiedafter maintainer design sign-off. Docs-only (no code) — implementation follows as separate phased slices, per the RFC process.RFC 0039 — Inbound trace-context propagation
Ourios's SERVER spans (RFC 0038) are currently trace roots — they never read the incoming W3C
traceparent, so a caller's trace stops at the boundary. This installs a globalTraceContextPropagatorand, at each ingress (ingest logs,POST /v1/query, MCP tool spans), extracts the caller's context andset_parents the span.tokio::spawn, but the carrier only exists before it — so theContextis extracted in the handler and carried into the closure (mirrors RFC 0038.3's span-context hand-off).[dev-dependencies]-only in ingester/server today → promoted to real deps.Given/When/Thencriteria mapped to the RFC 0038 test harnesses.RFC 0040 — DataFusion operator instrumentation
Deepens the flat
POST /v1/queryspan into an operator tree: one child span perExecutionPlannode, reconstructed post-hoc from the finished plan.BaselineMetricsrecords genuine wall-clockStart/EndTimestampper operator (reduced byaggregate_by_nameto earliest-start/latest-end), so the spans carry real bounds — option (a), not the synthetic-timing compromise first feared.ourios-df-otel(deps:datafusion+opentelemetryonly) — the isolation is what lets it lift todatafusion-contribasdatafusion-opentelemetry(the RFC 0038 §7 give-back). This RFC is the §7 authorization for the new crate.accumulate_scan_statsplan-tree walk; emission uses the raw OTel span builder (#[instrument]can't backdate); nodes without metrics are skipped, not faked.§3 invariant / hazard note
No pillar or on-disk change. RFC 0040 creates a new crate (§7 → this RFC) and both extend the traces pillar (RFC 0038). H6 preserved: no DataFusion type crosses the query boundary — the operator spans are an internal side-effect. OTel signal names go through the MCP + weaver, not guessed.
Renders under
mdbook build; both listed inSUMMARY.md.Summary by CodeRabbit