Skip to content

feat!: add global toggle for SQL trace-context annotation (default off) - #156

Merged
nicolasburtey merged 4 commits into
mainfrom
feat/sql-annotation-toggle
Jul 22, 2026
Merged

feat!: add global toggle for SQL trace-context annotation (default off)#156
nicolasburtey merged 4 commits into
mainfrom
feat/sql-annotation-toggle

Conversation

@nicolasburtey

@nicolasburtey nicolasburtey commented Jul 22, 2026

Copy link
Copy Markdown
Member

Adds sql_commenter::set_annotation_enabled(bool)disabled by default (opt-in).

While disabled (the default), annotate_sql is a zero-cost pass-through, so OneTimeExecutor delegates statements untouched: full sqlx prepared-statement reuse for all traffic. Calling set_annotation_enabled(true) once at process startup restores the traceparent annotation introduced in #154.

Motivation: annotated statements are executed persistent(false) — a server-side parse + plan per execution. Under stress-test load (obix outbox checkpoint churn ≈ 250 UPDATE job_executions/s) that cost is significant; this makes annotation an explicit choice and gives operators a config lever to reclaim the cost without recompiling.

lana-bank wires it to tracing.annotate_sql_statements in lana.yml (GaloyMoney/lana-bank#7452).

Tests: annotation_disabled_by_default_and_toggleable covers the default and the toggle; a mutex serializes toggle-sensitive tests since the flag is global. The trace_annotation integration test opts in explicitly.


Note

Medium Risk
Breaking default: annotation is off unless configured, changing observability and DB execution characteristics for existing deployments that relied on implicit annotation.

Overview
SQL traceparent comments are now opt-in via sql_commenter::set_annotation_enabled / annotation_enabled, backed by a process-wide atomic flag that defaults to disabled.

When the toggle is off, annotate_sql returns the original SQL immediately (Cow::Borrowed), so executors keep full prepared-statement reuse and avoid per-execution parse/plan cost. Enabling it at startup restores sampled-span annotation behavior from the prior feature.

Tests gain a mutex-serialized AnnotationEnabled helper for the global flag, a case for default-off plus toggle, and the Postgres integration test explicitly calls set_annotation_enabled(true) before exercising the end-to-end path.

Reviewed by Cursor Bugbot for commit e724cdf. Bugbot is set up for automated code reviews on this repo. Configure here.

set_annotation_enabled(false) makes annotate_sql a zero-cost
pass-through, restoring prepared-statement reuse for all traffic at
the cost of losing statement-level trace correlation. Intended to be
set once at process startup from application configuration.
@nicolasburtey
nicolasburtey marked this pull request as ready for review July 22, 2026 15:51
Annotation is now opt-in: annotate_sql is a pass-through until
set_annotation_enabled(true) is called at process startup. Tests that
exercise the annotated path enable it explicitly.
@nicolasburtey nicolasburtey changed the title feat: add global toggle for SQL trace-context annotation feat!: add global toggle for SQL trace-context annotation (default off) Jul 22, 2026
Comment thread src/sql_commenter.rs
unsampled_span_is_not_annotated passed vacuously once annotation
became disabled by default. Enable the flag explicitly so the test
still verifies the sampling gate: an un-sampled span is never
annotated even when annotation is on.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit a028246. Configure here.

Comment thread src/sql_commenter.rs
Guard the global flag with an RAII helper that restores the default
(disabled) on drop, and tolerate mutex poisoning from a previously
panicked test instead of unwrap()-ing the lock.
@nicolasburtey
nicolasburtey merged commit 59d1ac5 into main Jul 22, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant