Conversation
Wrap `Planner::optimize_expr` in a `filter_optimize` tracing span at trace level so the cost of simplifying and coercing a filter expression shows up in a trace alongside the scan it belongs to. The span uses `skip_all`, so the expression itself is not recorded.
Wrap `execute_plan` and `analyze_plan_with_context` in debug-level tracing spans. `execute_plan` is synchronous and returns a stream, so its span covers session-context creation, coalescing of a multi-partition plan and `plan.execute()` (stream construction), and closes before the caller polls the stream. Spans opened by operators while the stream is polled are not children of it. `analyze_plan_with_context` is async and drives the stream to completion inside the call, and already re-parents the operators' spans under `Span::current()` via `TracedExec`; the new span gives that parent a name. It is attached to `analyze_plan_with_context`, which holds the body, rather than to the thin `analyze_plan` wrapper, so callers of either entry point are covered; it is named `analyze_plan` so the span name matches the public entry point. Both spans use `skip_all`, so the plan and options are not recorded as fields.
amunra
marked this pull request as ready for review
September 16, 2026 14:52
Cover the three spans this PR adds. `Planner::optimize_expr` opens `filter_optimize` at trace level and `execute_plan` opens `execute_plan` at debug level, both asserted with `tracing-mock`, the harness `lance-io` already uses. It matches a strictly ordered queue against every notification it is shown, so each test filters the subscriber down to the single span it asserts on. `analyze_plan_with_context` opens `analyze_plan` at debug level and runs the plan underneath it. `TracedExec` re-parents both the plan's `execute` and every poll of the stream `execute` returns, so the test covers both, using a small recording `Layer` read back after the subscriber is uninstalled. An ordered queue would instead have to spell out each enter and exit of the analyze span, pinning down when `AnalyzeExec` spawns its input task; it would also report a lost parent as an out-of-order enter, and a panic inside a tracing callback aborts the whole test binary. `execute_plan` gets no parent assertion: it returns before the stream is polled, so operator spans opened during polling are not its children.
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The prior coverage finding is fixed: this revision verifies all three span names and levels, plus analyze_plan parenting for both plan execution and stream polling. The instrumentation remains narrowly scoped to tracing with no API or format changes.
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.
Adds
tracingspans around the three DataFusion entry points inlance-datafusionthat sit between a caller's request and the operators that do the work: filter optimization, plan execution setup, and plan analysis.What is missing
execute_plan,analyze_plan_with_contextandPlanner::optimize_expropen no span of their own. The time spent simplifying and coercing a filter expression, building the session context and constructing the execution stream is attributed to whatever span the caller happens to have open, andanalyze_plan's re-parenting of operator spans throughTracedExechangs them off whatever span the caller has open rather than a query-specific one.What this does
Planner::optimize_expris wrapped in afilter_optimizespan attracelevel.execute_planis wrapped in anexecute_planspan atdebuglevel.execute_planis synchronous and returns a stream, so the span covers session-context creation, coalescing of a multi-partition plan andplan.execute()(stream construction), and closes before the caller polls the stream. Spans opened by operators while the stream is polled are not children of it.analyze_plan_with_contextis wrapped in a span namedanalyze_planatdebuglevel. This function is async, drives the stream to completion inside the call, and already wraps the plan inTracedExec::new(plan, Span::current())so thatAnalyzeExec's per-partition tasks report under the caller's span; the new span gives that parent a name, so the whole analysis, including the operators' own spans, sits underanalyze_plan. The attribute sits onanalyze_plan_with_context, which holds the body, rather than on the thinanalyze_planwrapper, so callers of either entry point are covered; the span is named after the public entry point.All three spans use
skip_all: the plan, the filter expression and the execution options are not recorded as span fields, so enabling the spans adds a fixed per-call cost and never formats a plan tree.Why this design
#[instrument]attributes on the existing functions leave the signatures untouched and let the span levels be tuned independently: filter optimization is per-query and cheap, so it istrace; execution setup and analysis are the natural top-level units of a query trace, so they aredebug. Instrumenting the stream returned byexecute_plan(so that polling is covered too) is deliberately out of scope here and is a separate decision.Testing
The spans have no observable behaviour to assert, so there are no new tests.
cargo test -p lance-datafusionandcargo clippy --all --tests --benches -- -D warningspass.Compatibility
No API, format or dependency changes;
lance-datafusionalready depends ontracing.Tracking: Ported from rerun-io#71.