Skip to content

fix(subscriptions): link to the restored trace context, don't parent - #570

Open
Inok wants to merge 1 commit into
Eventuous:devfrom
Inok:fix/subscription-trace-links
Open

fix(subscriptions): link to the restored trace context, don't parent#570
Inok wants to merge 1 commit into
Eventuous:devfrom
Inok:fix/subscription-trace-links

Conversation

@Inok

@Inok Inok commented Aug 13, 2026

Copy link
Copy Markdown

Consume spans were parented to the tracing context restored from the event's
metadata, making each consume a child of the append that produced the event.
That context is durable data on the event, so every redelivery re-joined the
same trace — no live root to end it, no sampling decision left to take. A hot
resubscribe loop grew one trace to 251MB and OOM-killed a shared Tempo twice.

The consume span now roots its own trace and carries an ActivityLink to the
restored context, which is also what the OpenTelemetry messaging conventions
prescribe for an asynchronous "process" operation. Tags are unchanged.
Parent-child is kept where the causality is in-process: an ambient Eventuous
activity, or the activity stored for the async handling path. The SignalR
client had the same bug and is fixed alongside.

Three defects found alongside, each wrong on its own:

  • ToActivityContext returned a zeroed context instead of null, so both
    callers' null checks never fired.
  • AddTracingMeta compared a 32-char trace id to a 16-char constant, so its
    empty-id guard never fired and all-zero contexts were written to events.
  • AddEventuousTracing called SetSampler, replacing the application's sampler
    for the whole provider; its drop branch matched no activity that exists, so it
    only ever returned RecordAndSample.

Breaking change for consumers of the traces

Navigating from a producer span to its consumers by parentage no longer works —
follow links instead. Applications that relied on AddEventuousTracing to
sample everything now need an explicit AlwaysOnSampler.

Testing

18 new unit tests across subscriptions, SignalR and tracing metadata; each fix
red-checked by reverting it. TracesTests verified against a real KurrentDB.
Suites: subscriptions 50/50, core 29/29, application 21/21, gateway 10/10,
SignalR 16/16, analyzers 1/1, Sqlite 27/27.

Docs still to update in eventuous-docs — not in this repo.

🤖 Generated with Claude Code

The consume span was made a child of the append that produced the event, using
the tracing context restored from the event's metadata. That context is durable
data on the event, so every redelivery re-joined the same trace, with no live
root to end it and no sampling decision left to take. A hot resubscribe loop
grew one trace to 251MB and OOM-killed a shared Tempo twice.

The consume span now roots its own trace and links to the restored context.
Tags are unchanged. Parent-child is kept where the causality is in-process: an
ambient Eventuous activity, or the activity stored for the async path. The
SignalR client had the same bug and is fixed alongside.

Rooting takes two steps, because .NET has no parentless argument and binds the
parent from Activity.Current at Start() — which on the async path happens far
down the pipe. Activity.Current is suppressed at creation, and the span is
pinned to a zero parent span id so a late Start cannot re-parent it. The trace
id is the sampler's own where it read one (only ratio-based samplers do) and
generated otherwise, since an unstarted activity reports all zeros.

Three defects found alongside, each wrong on its own:

- ToActivityContext returned a zeroed context instead of null, so both callers'
  null checks never fired.
- AddTracingMeta compared a 32-char trace id to a 16-char constant, so its
  empty-id guard never fired and all-zero contexts were written to events.
- AddEventuousTracing called SetSampler, replacing the application's sampler for
  the whole provider; its drop branch matched no activity that exists, so it
  only ever returned RecordAndSample.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@Inok
Inok marked this pull request as ready for review August 13, 2026 22:40
@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Fix subscription/SignalR consume tracing to link remote context instead of parenting

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

Grey Divider

AI Description

• Root consumer spans in new traces and link to producer context to avoid unbounded traces.
• Stop persisting/restoring unusable (all-zero) tracing metadata by validating IDs end-to-end.
• Remove global sampler override from AddEventuousTracing and add focused unit/integration coverage.
Diagram

graph TD
  P["Producer span"] --> ES[("Event store") ] --> M["Event metadata (trace/span)"] --> C["Subscription consume"] --> A["Consume span (new trace)"] --> L["ActivityLink to producer"]
  M --> SR["SignalR client consume"] --> SA["SignalR consume span (new trace)"] --> SL["ActivityLink to producer"]
  OT["OTel TracerProvider"] --> SRC["Eventuous source registered"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Keep parent-child and rewrite metadata on redelivery
  • ➕ Preserves legacy navigation from producer to consumer via parent relationships
  • ➕ Fewer tracing model changes for existing dashboards
  • ➖ Not generally possible/reliable: redelivery/replay can happen outside application control
  • ➖ Still risks trace growth if any path reuses persisted context unchanged
  • ➖ Violates OTel messaging guidance for async processing (should be a link, not a parent)
2. Stop persisting trace context entirely (use correlation only)
  • ➕ Eliminates any chance of rejoining a durable trace on replay
  • ➕ Simplifies consumer-side logic
  • ➖ Loses cross-process causality in traces (producer→consumer correlation)
  • ➖ Harder troubleshooting compared to linked context
3. Rely on OpenTelemetry propagators and semantic conventions end-to-end
  • ➕ Aligns fully with OTel extraction/injection patterns
  • ➕ Potentially reduces custom metadata handling over time
  • ➖ Requires broader API and integration changes across producers/consumers
  • ➖ More migration work than the targeted fix in this PR

Recommendation: The PR’s approach (root the consume span and add an ActivityLink to the restored producer context) is the best fit: it prevents unbounded trace growth on replays/redeliveries while retaining causality, and it matches OpenTelemetry’s async messaging conventions. The additional fixes (null vs default context, all-zero ID guards, and removing provider-wide sampler override) close correctness gaps that would otherwise keep producing unusable or misleading tracing data.

Files changed (10) +551 / -52

Bug fix (5) +121 / -41
MetadataExtensions.csValidate tracing meta before persisting to event metadata +2/-4

Validate tracing meta before persisting to event metadata

• Updates AddTracingMeta to skip writing tracing IDs when the supplied TracingMeta is invalid (missing or all-zero). Removes the prior incorrect empty-id constant check to avoid persisting unusable contexts.

src/Core/src/Eventuous.Diagnostics/MetadataExtensions.cs

TracingMeta.csTreat all-zero tracing IDs as invalid and return null context +20/-6

Treat all-zero tracing IDs as invalid and return null context

• Strengthens TracingMeta validity checks to reject missing or all-zero trace/span IDs. Changes ToActivityContext to return null (not default/zeroed ActivityContext) when metadata is unusable or parsing fails, restoring callers’ null-guard behavior.

src/Core/src/Eventuous.Diagnostics/Tags/TracingMeta.cs

SubscriptionActivity.csRoot consume spans and link to remote producer context +71/-10

Root consume spans and link to remote producer context

• Replaces parenting to restored remote context with creating a linked root activity (new trace) to prevent trace amplification on redelivery/replay. Preserves parent-child only for in-process causality (ambient Eventuous activity or stored async-path activity) and adds safeguards against late Start() re-parenting.

src/Core/src/Eventuous.Subscriptions/Diagnostics/SubscriptionActivity.cs

TracerProviderBuilderExtensions.csStop overriding provider sampler in AddEventuousTracing +3/-11

Stop overriding provider sampler in AddEventuousTracing

• Removes the custom PollingSampler and the SetSampler call so AddEventuousTracing only registers the Eventuous ActivitySource. This avoids globally replacing the application’s sampler and keeps sampling policy owned by the host application.

src/Diagnostics/src/Eventuous.Diagnostics.OpenTelemetry/TracerProviderBuilderExtensions.cs

TypedStreamSubscription.csFix SignalR consume tracing to link remote context +25/-10

Fix SignalR consume tracing to link remote context

• Makes StartTraceActivity internal for testing and changes it to create a root consume span with an ActivityLink to the restored producer context, suppressing ambient Activity.Current to avoid unintended parenting. Returns null when no valid tracing context exists or metadata is malformed.

src/SignalR/src/Eventuous.SignalR.Client/TypedStreamSubscription.cs

Tests (4) +427 / -11
SubscriptionActivityTests.csAdd unit tests for subscription consume linking/rooting behavior +252/-0

Add unit tests for subscription consume linking/rooting behavior

• Introduces comprehensive tests verifying: linked root behavior, distinct traces on redelivery, correct sampler/recording flags, correct parenting for in-process cases, and avoidance of foreign ambient parenting or all-zero metadata links.

src/Core/test/Eventuous.Tests.Subscriptions/SubscriptionActivityTests.cs

TracingMetadataTests.csAdd unit tests for tracing metadata persistence rules +72/-0

Add unit tests for tracing metadata persistence rules

• Adds tests ensuring started activities persist real trace/span IDs, unstarted activities do not persist all-zero IDs, and existing metadata is not overridden.

src/Core/test/Eventuous.Tests/TracingMetadataTests.cs

ProducerTracesTests.csUpdate integration test to assert links instead of parentage +29/-11

Update integration test to assert links instead of parentage

• Renames and adjusts the KurrentDB trace propagation test to assert that the subscription activity roots a new trace and contains an ActivityLink to the producer’s trace/span. Captures started activities to locate and inspect the subscription span reliably.

src/KurrentDB/test/Eventuous.Tests.KurrentDB/ProducerTracesTests.cs

ConsumeTracingTests.csAdd unit tests for SignalR consume trace linking/rooting +74/-0

Add unit tests for SignalR consume trace linking/rooting

• Adds tests verifying SignalR consume spans do not parent to restored context, create links correctly, produce distinct traces on redelivery, and safely return null when metadata is missing or invalid.

src/SignalR/test/Eventuous.Tests.SignalR/ConsumeTracingTests.cs

Other (1) +3 / -0
Eventuous.SignalR.Client.csprojExpose internals to SignalR tracing tests +3/-0

Expose internals to SignalR tracing tests

• Adds InternalsVisibleTo for Eventuous.Tests.SignalR to enable direct testing of internal tracing helpers.

src/SignalR/src/Eventuous.SignalR.Client/Eventuous.SignalR.Client.csproj

@qodo-free-for-open-source-projects

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Tip of the day
💡 Did you know, you can type 'qodo, fix this' on a finding and the fix lands right on your PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

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