feat!: annotate SQL statements with OTel trace context via AtomicOperation - #154
Conversation
Add opt-in sqlcommenter-style trace correlation for all statements executed through es-entity. New sql_commenter module extracts the W3C traceparent from the current span (tracing-context feature) and appends it as a SQL comment. A TraceAnnotatingExecutor rewrites only the statement text at execution time; bind arguments and row mapping flow through unchanged, so sqlx::query! compile-time verification is fully preserved and no macro codegen types change. The wrapper is applied centrally: - OneTimeExecutor fetch methods (covers all es_query! reads) - generated write paths (persist, create, update, delete, nested) When there is no active span context the original query is passed through untouched (zero overhead). Annotated statements are marked non-persistent since the unique comment would otherwise thrash the per-connection prepared statement cache. The comment is retained in pg_stat_statements' representative query text and in Postgres logs, so a statement observed server-side can be matched to a distributed trace: SELECT * FROM pg_stat_statements WHERE query LIKE '%<trace_id>%'; Includes an integration test proving the traceparent of the active span is visible in pg_stat_activity while a query is in flight.
Alternative to per-call-site executor wrapping: annotation is centralized in the AtomicOperation trait and the existing OneTimeExecutor type instead of being applied at every macro codegen site. - AtomicOperation is split: implementors now provide connection() (raw &mut PgConnection); as_executor() becomes a provided method returning a OneTimeExecutor that annotates every statement with the current sampled span's traceparent SQL comment. - OneTimeExecutor implements sqlx::Executor and hosts the annotation logic; the TraceAnnotatingExecutor wrapper type is removed. - Macro codegen is reverted to plain op.as_executor() - generated code is unchanged from main. - HookOperation inherits the annotating executor, so SQL executed in commit hooks (pre_commit) is annotated automatically. - Annotation is gated on the span's sampled flag: unsampled spans are never exported, so annotating them would cost without benefit. Trade-off: an annotated statement's text is unique per span, so it can never be served from sqlx's per-connection prepared statement cache. Annotated statements run with persistent(false) (unnamed statement), bypassing the cache rather than thrashing it, at the cost of a server-side parse + plan per sampled execution. Unsampled traffic keeps full prepared-statement reuse. BREAKING CHANGE: AtomicOperation implementors must implement connection() instead of as_executor(); as_executor() now returns OneTimeExecutor<&mut PgConnection> (still an sqlx::Executor, so call sites passing it to query execution compile unchanged). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Alternative implementation of GaloyMoney/obix#79 that keeps the changes out of the macro code. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4f6be80. Configure here.
| Box::pin(async move { | ||
| let q = rebuild_annotated(annotated.as_str(), query)?; | ||
| self.executor.fetch_optional(q).await | ||
| }) |
There was a problem hiding this comment.
Annotation ignores post-hoc span instrument
Medium Severity
annotated_sql / current_traceparent run synchronously when fetch_many or fetch_optional is called, before the returned future or stream is polled. Attaching a span with .instrument(span) after .execute(...) / .fetch_*(...) therefore sees no active context and silently skips the traceparent comment, even though the span is current while the query runs. #[instrument] on an enclosing async function still works because the span is entered before those calls.
Reviewed by Cursor Bugbot for commit 4f6be80. Configure here.
There was a problem hiding this comment.
Acknowledged — the observation is technically correct: the annotation decision runs at call time, before the returned future/stream is polled, so .instrument(span) attached directly to a fetch future misses the traceparent comment.
Marking as won't-fix:
- Every generated repo method uses
#[tracing::instrument]attributes, and the supported pattern for manual spans is instrumenting an enclosing async block (seetests/trace_annotation.rs) — in both cases the span is active at call time and annotation works. - The failure mode is graceful: a post-hoc-instrumented fetch future just loses the SQL comment (observability gap only); query execution is unaffected.
- Capture-at-call-time matches common tracing convention (cf.
Span::current()captured in constructors), and deferring the check into the future would force aBox::pinonto the unannotated fast path, which currently returns the inner future directly.
If a real downstream use case for post-hoc instrumentation of raw fetch futures shows up, the fix is small (~20 lines: move the annotated_sql() check inside the returned future/stream) and can land as a follow-up.


Summary
Alternative implementation of #153 (sqlcommenter-style trace correlation), per review discussion: instead of wrapping the executor at every macro call site, annotation is centralized in
AtomicOperationand the existingOneTimeExecutor— no new executor type, zero macro codegen changes.Every statement executed through
AtomicOperation::as_executor()orIntoOneTimeExecutorcarries the active sampled span's W3Ctraceparentas a trailing SQL comment:Observable in
pg_stat_activityand Postgres logs (slow query log,auto_explain, lock waits) for exact per-execution correlation;pg_stat_statementsretains the comment of the first execution per query id (comments are excluded from the query-id jumble), yielding an exemplar trace per statement shape.Design
AtomicOperationis split:connection()(required) — raw&mut PgConnection; replacesas_executor()as the method implementors provideas_executor()(provided) — returnsOneTimeExecutor<&mut PgConnection>, which annotatesOneTimeExecutornow implementssqlx::Executorand hosts the annotation logic. It rewrites statement text only viaExecute::sql()+take_arguments(); bind arguments and row mapping flow through unchanged, sosqlx::query!compile-time verification is fully preserved.main— generated code still calls plainop.as_executor()and gets annotation through the trait. All 85+ token tests pass unmodified.HookOperationinherits the annotating executor, so SQL executed inside commit hooks (pre_commit) is annotated automatically — no per-hook opt-in.Trade-off: sampled statements bypass the prepared statement cache
The traceparent comment makes each annotated statement's text unique — it can never match sqlx's per-connection prepared statement cache. Annotated statements therefore run with
persistent(false)(the unnamed statement): this bypasses the cache instead of thrashing it with single-use named statements, at the cost of a server-side parse + plan per sampled execution. Because annotation is sampled-gated, all unsampled traffic keeps full prepared-statement reuse. The trade-off is inherent to SQL-comment correlation — sampling bounds it; it cannot be eliminated for sampled traffic.Compared to #153
TraceAnnotatingExecutor,annotate_executorop.as_executor()SQL…-00, unlookupable)feat!)Breaking change
AtomicOperationimplementors must implementconnection()instead ofas_executor(). Org-wide code search found no implementors outside this crate, and every downstream call site observed usesop.as_executor()in executor-argument position (.execute(...)/.fetch_*(...)), which compiles unchanged sinceOneTimeExecutorimplementssqlx::Executor. Raw connection access remains available viaconnection()andDbOp::tx_mut().Test plan
--all-featuresagainst Postgres — every generated read/write path now executes through the newExecutorimpl, proving transparencytests/trace_annotation.rsproves the traceparent reachespg_stat_activity, now exercising the realDbOp::as_executor()path used by all generated writesnix flake checkgreen (fmt, clippy--all-features -D warnings, deny, audit)nix run .#nextestruns without--all-features, so thetracing-context-gated integration test is compiled out in CI — this also applies to feat: annotate SQL statements with OTel trace context #153; verified locally instead🤖 Generated with Claude Code
Note
Medium Risk
Breaking trait change for custom
AtomicOperationtypes, and sampled traffic pays extra parse/plan cost due to non-persistent queries; core persistence paths are affected but annotation is gated on sampling.Overview
Breaking:
AtomicOperationimplementors now provideconnection()instead ofas_executor(); the trait supplies a defaultas_executor()that wraps the connection inOneTimeExecutor.Adds
sql_commenterto append sqlcommenter-styletraceparentcomments whentracing-contextis enabled and the active span is sampled; unsampled or missing context leaves SQL unchanged.OneTimeExecutorimplementssqlx::Executorand centralizes annotation: sampled statements get rewritten text withpersistent(false)(bypassing sqlx’s prepared-statement cache); binds and row mapping are unchanged. Generated macro code still callsop.as_executor()with no codegen edits.Direct use of
connection()does not annotate—callers should useas_executor()for trace correlation (including commit hooks viaHookOperation).Tests cover unit sampling behavior and an integration check that annotated SQL appears in
pg_stat_activity.Reviewed by Cursor Bugbot for commit 4f6be80. Bugbot is set up for automated code reviews on this repo. Configure here.