feat: add commenter_for_nonrecording_spans option to enable SQLCommenter for non-recording spans

[Elyahou]

Description

Add commenter_for_nonrecording_spans option to SQLAlchemy and DBAPI instrumentations, enabling SQLCommenter to add SQL comments (including traceparent) for non-recording (unsampled) spans.

Motivation: By default, SQLCommenter only adds comments when span.is_recording() is True. This means unsampled spans (e.g., when using a sampling rate < 100%) don't get SQL comments. However, some users want traceparent propagated in SQL comments for all requests regardless of sampling decision, enabling database-side correlation with distributed traces.

Changes:

• Added commenter_for_nonrecording_spans parameter (default: False) to both SQLAlchemy and DBAPI instrumentations
• When enabled, SQL comments are added even for non-recording spans
• Span attributes are still only set when span.is_recording() is True (existing behavior preserved)

Type of change

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Added unit tests for both instrumentations:

• test_commenter_for_nonrecording_spans_disabled_by_default - Verifies SQL comments are NOT added for non-recording spans when flag is False (default)
• test_commenter_for_nonrecording_spans_enabled - Verifies SQL comments ARE added for non-recording spans when flag is True

[tammy-baylis-swi]

Hi again @Elyahou , thanks for this!

The overall approach to changing the instrumentors looks reasonable. The application of concepts needs a bit of fine-tuning though and apologies if my Slack message was confusing.

"Sampled" is whether span data is exported or not, i.e. the 01 vs 00 traceflags. iiuc we want something like commenter_for_nonsampled_spans.

"Recording" is whether span data is kept through instrumentation and propagation and corresponding metrics are recorded, whether sampled or not. If not recorded, telemetry is dropped completely.

So I think if commenter_for_nonsampled_spans is True, there would be sqlcomment appended for both 01 and 00. But no sqlcomment appended if not recording because there won't be a valid trace id anyway.

[Elyahou]

Hi! Thanks for the feedback. I investigated this thoroughly and want to clarify a few things:

Non-recording spans DO have valid trace IDs

I tested this directly:

from opentelemetry.sdk.trace import TracerProvider
from opentelemetry.sdk.trace.sampling import ALWAYS_OFF

provider = TracerProvider(sampler=ALWAYS_OFF)
span = provider.get_tracer(__name__).start_span('test')

print(f'is_recording: {span.is_recording()}')
print(f'trace_id: {span.get_span_context().trace_id:032x}')
print(f'span_id: {span.get_span_context().span_id:016x}')
print(f'is_valid: {span.get_span_context().is_valid}')

Output:

Property	Value	Status
is_recording	False
trace_id	173482935693c85a55f29d2678655838	✅ Valid
span_id	ecd82b622a62fbdb	✅ Valid
is_valid	True	✅

Non-recording spans maintain valid trace context for propagation, even though they're not exported.

The original code used is_recording(), not trace_flags.sampled

The existing logic before my change:

if span.is_recording():
    if self.enable_commenter:
        # add SQL comment

There was no logic checking trace_flags.sampled.

With standard samplers like TraceIdRatioBasedSampler:

• Sampled → RECORD_AND_SAMPLE → is_recording()=True
• Unsampled → DROP → is_recording()=False

Naming proposition

I propose renaming commenter_for_nonrecording_spans → commenter_for_all_spans

This makes it clearer that it enables SQL comments regardless of the sampling decision. What do you think?