Skip to content

Make shared OpenTelemetry instrumentation options thread-visible#1207

Merged
mtmk merged 1 commit into
release/3.0from
otel-instrumentation-options-volatile
Jul 1, 2026
Merged

Make shared OpenTelemetry instrumentation options thread-visible#1207
mtmk merged 1 commit into
release/3.0from
otel-instrumentation-options-volatile

Conversation

@mtmk

@mtmk mtmk commented Jul 1, 2026

Copy link
Copy Markdown
Member

The shared NatsInstrumentationOptions.Default is configured once at startup, but its Filter, Enrich, and SpanDestinationNameFormatter callbacks are read on the publish/subscribe path from other threads. Back the properties with volatile fields so a configuration change applied after traffic has started is visible to those readers rather than depending on incidental memory barriers. No new test: this is a visibility guarantee and the existing configure round-trip test already covers get/set behavior.

The Default instance is configured at startup but its callbacks are read on
the publish/subscribe path from other threads. Back the properties with
volatile fields so a configuration change made after traffic has started is
observed by those readers instead of relying on incidental barriers.
@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Claude finished @mtmk's task in 1m 28s —— View job


Code Review

  • Gather context
  • Read changed file and usages
  • Analyze diff
  • Post review findings

The change is correct and well-targeted. A few observations:

Memory model correctness

volatile on reference-typed fields (Func<>, Action<>) gives you:

  • Atomic reads/writes (pointer-sized, always true for reference types on .NET)
  • Acquire semantics on reads and release semantics on writes

That is exactly what's needed for the "configure once at startup, read on many hot-path threads" pattern. The fix is sound on both x86 and ARM.

Hot path reads are safe

All three properties are read in Telemetry.StartSendActivity, Telemetry.StartReceiveActivity, and NatsConnection.SpanDestinationName — all on the message publish/subscribe path. With the volatile backing fields, the acquiring reads in those sites will see any release write done during startup configuration.

FilterSubjects compound read-modify-write

NatsInstrumentationOptionsExtensions.FilterSubjects does:

var previous = options.Filter;   // volatile read
options.Filter = ctx => { ... }; // volatile write

This is a read-modify-write that isn't atomic. If two threads call FilterSubjects concurrently on the same instance, one closure could be lost. This is a pre-existing concern (not introduced by this PR) and is acceptable given that FilterSubjects is a startup-configuration call. No action needed here, but worth noting if the API is ever documented for concurrent use.

volatile vs Interlocked

volatile is the right choice here — Interlocked.Exchange/CompareExchange would provide sequentially-consistent fences but would be heavier and unnecessary for simple assignment. The volatile keyword is idiomatic for this pattern.

No issues found. The fix is minimal, correct, and appropriately scoped.

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown

@scottf scottf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

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.

2 participants