feat(df-otel): post-hoc ExecutionPlan -> OTel operator span crate (RFC 0040 slice 1) - #632
Conversation
…C 0040 slice 1) Prototyped and measured both post-hoc reconstruction and adopting datafusion-contrib/datafusion-tracing before building: the latter drops every operator span on Ourios's real multi-partition plans (RepartitionExec's internal spawns aren't covered by its join-set tracer hook) and emits string-typed attributes, not the normative datafusion.operator.* table. record_plan_spans walks a finished physical plan and emits one child span per BaselineMetrics-backed node using its real StartTimestamp/EndTimestamp wall clock, skipping untimed nodes and re-parenting their children instead of inventing a timeline. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qtny6z6cA74xPZa4qRhk4F Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
|
Warning Review limit reached
Next review available in: 52 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 (1)
📝 WalkthroughWalkthroughThe PR adds the ChangesOpenTelemetry instrumentation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ParentContext
participant record_plan_spans
participant ExecutionPlan
participant OpenTelemetryTracer
ParentContext->>record_plan_spans: provide sampled parent context
record_plan_spans->>ExecutionPlan: read metrics and children
record_plan_spans->>OpenTelemetryTracer: create timed operator span
record_plan_spans->>ExecutionPlan: recurse with updated parent context
OpenTelemetryTracer-->>ParentContext: record reconstructed span tree
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
cargo about's attribution list is keyed off workspace members; ourios-df-otel (RFC 0040 slice 1) wasn't in it yet. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qtny6z6cA74xPZa4qRhk4F Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
There was a problem hiding this comment.
Pull request overview
Adds a new, dependency-light crate to emit OpenTelemetry operator spans from a completed DataFusion physical plan (post-hoc), and updates RFC 0040 to match the implemented API constraints and settled design decisions.
Changes:
- Introduce
crates/ourios-df-otelwithrecord_plan_spansthat walks an executedExecutionPlanand emits one span per timed (BaselineMetrics) node using Start/EndTimestamp wall-clock bounds. - Update RFC 0040 to correct the
record_plan_spanssignature and document the settled approach vs.datafusion-contrib/datafusion-tracing. - Register the new crate in the workspace (and lockfile).
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/rfcs/0040-datafusion-operator-instrumentation.md | Updates the RFC signature bounds and documents findings/decisions from prototype evaluation. |
| crates/ourios-df-otel/src/lib.rs | Implements post-hoc plan-walk span emission plus unit tests around metrics mapping and untimed-wrapper behavior. |
| crates/ourios-df-otel/Cargo.toml | Adds the new crate manifest with minimal deps (DataFusion + OTel) and test-only SDK deps. |
| Cargo.toml | Adds crates/ourios-df-otel to workspace members. |
| Cargo.lock | Records the new workspace crate and its dependencies. |
Comments suppressed due to low confidence (2)
crates/ourios-df-otel/src/lib.rs:113
- These
as i64casts can wrap on overflow, which would record negative values into span attributes. Even if it’s unlikely in practice, a wrapped negative is always wrong telemetry; preferi64::try_from(...)and omit the attribute if it doesn’t fit.
MetricValue::OutputRows(count) => {
attrs.push(KeyValue::new(ATTR_OUTPUT_ROWS, count.value() as i64));
}
MetricValue::ElapsedCompute(time) => {
attrs.push(KeyValue::new(ATTR_ELAPSED_COMPUTE, time.value() as i64));
crates/ourios-df-otel/src/lib.rs:122
- Like the other metric mappings,
pruned()/matched()are cast withas i64and can wrap to negative values on overflow. Usei64::try_fromand skip attributes that don’t fit to avoid incorrect telemetry.
MetricValue::PruningMetrics {
name,
pruning_metrics,
} if name == ROW_GROUPS_PRUNED_STATISTICS => {
attrs.push(KeyValue::new(
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
crates/ourios-df-otel/src/lib.rs:112
- Direct
as i64casts fromusizecan wrap on overflow and yield negative OpenTelemetry attributes. Usingi64::try_fromavoids silent corruption and also makes thecast_possible_wrapallowance unnecessary once all casts are removed.
match metric.value() {
MetricValue::OutputRows(count) => {
attrs.push(KeyValue::new(ATTR_OUTPUT_ROWS, count.value() as i64));
}
MetricValue::ElapsedCompute(time) => {
…casts Copilot review on RFC 0040 slice 1 (PR #632): - timed_bounds now explicitly reduces via earliest-start/latest-end and treats an inverted end < start as untimed (skip + re-parent), rather than trusting aggregate_by_name to have already collapsed to one instance and overwriting on each iteration. - node_attributes converts usize -> i64 via try_from, omitting the attribute on overflow instead of wrapping to a silently negative value; the cast_possible_wrap allow is gone because there's no wrapping cast left. - The skip/reparent test pins an explicit AlwaysOn sampler instead of relying on the SDK default, since record_plan_spans gates on sampling. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qtny6z6cA74xPZa4qRhk4F Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@crates/ourios-df-otel/src/lib.rs`:
- Around line 90-128: Add unit tests covering the invalid-data policies in
timed_bounds and attr: verify timed_bounds returns None when the end timestamp
precedes the start timestamp, and verify attr returns None for a usize value
exceeding i64::MAX. Use the existing test conventions and target these symbols
directly without changing their production behavior.
🪄 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: ec61352a-1159-4e91-a750-1a4954da1918
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
Cargo.tomlTHIRD-PARTY-LICENSES.mdcrates/ourios-df-otel/Cargo.tomlcrates/ourios-df-otel/src/lib.rsdocs/rfcs/0040-datafusion-operator-instrumentation.md
CodeRabbit review on PR #632: the two invalid-data policies added in the prior fix (end < start treated as untimed; usize -> i64 overflow omits the attribute rather than wrapping) had no dedicated tests protecting them from regression. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qtny6z6cA74xPZa4qRhk4F Signed-off-by: Jens Holdgaard Pedersen <Jens@holdgaard.org>
Summary
crates/ourios-df-otelcrate:record_plan_spanswalks a finishedDataFusionphysical plan and emits one child OTel span perBaselineMetrics-backedExecutionPlannode, using the node's realStartTimestamp/EndTimestampwall-clock bounds (RFC 0040 §3.1–§3.3). Deliberatelydatafusion+opentelemetry-only, noourios-*deps, so it lifts cleanly to a standalonedatafusion-contribcrate later.datafusion-contrib/datafusion-tracing(a live-spanPhysicalOptimizerRule). The latter has a production-blocking bug on Ourios's real multi-partition plans —RepartitionExec's internaltokiospawns aren't covered by its join-set tracer hook (DataFusion 54.0.0), so every operator span is silently dropped unless partitioning is disabled — and its attributes are pretty-printed strings, not the normative typed schema this RFC specifies. RFC 0040 §7 is updated with these findings.docs/rfcs/0040-datafusion-operator-instrumentation.md: fixed the §3.2record_plan_spanssignature to thewhere T::Span: Send + Sync + 'staticbound theContextAPI actually requires (found while implementing — the bareT: Tracersketch doesn't compile), settled both remaining §7 open questions (crate-name/extraction, querier OTel deps), and added a short note on why backdated timestamps +SpanKind::Internalare OTel-spec-sanctioned (verified via the OTel MCP), not a workaround.Related
RFC:
docs/rfcs/0040-datafusion-operator-instrumentation.md(slice 1 of the implementation; querier wiring + acceptance-criteria tests land in later slices, mirroring RFC 0039's 4-slice split)Checklist
cargo fmt -p ourios-df-otel --checkcleancargo clippy -p ourios-df-otel --all-targets --all-features -- -D warningscleancargo test -p ourios-df-otel --all-features, 3/3 pass)CHANGELOG.mdupdated (this is a design/doc correction to an already-specified RFC, not a user-facing change yet)Note: this crate isn't wired into
ourios-querieryet — that's slice 3, once the weaver registry entries fordatafusion.operator.*(slice 2) exist. No behavior change to the running binary in this PR.Summary by CodeRabbit
New Features
Documentation
Chores