feat: Spotlight spec compliance - DSN-less mode, pipeline bypass, env var support#5302
feat: Spotlight spec compliance - DSN-less mode, pipeline bypass, env var support#5302mattico wants to merge 1 commit into
Conversation
… var support Refactor Spotlight from a transport-wrapping decorator into a standalone transport that intercepts envelopes in SentryClient before filtering/sampling/PII-redaction. - Allow SDK initialization without a DSN when Spotlight is enabled (Spotlight-only mode) - Spotlight bypasses BeforeSend, event processors, transaction processors, sampling, and PII redaction - Add SENTRY_SPOTLIGHT env var support with truthy/falsy/URL parsing and config precedence - Use full TransactionTracer for sampled-out transactions when Spotlight is enabled - Add NoOpTransport for DSN-less operation - Add explicit-set tracking for EnableSpotlight and SpotlightUrl options SendToSpotlight serializes the envelope synchronously on the calling thread before the fire-and-forget, producing an immutable byte[] snapshot. This eliminates the race where the main pipeline mutates the event concurrently with Spotlight serialization. A new ISpotlightTransport interface accepts pre-serialized bytes instead of Envelope objects, making the race structurally impossible; SpotlightHttpTransport now only handles the HTTP POST and backoff. Envelope creation is skipped entirely when SpotlightTransport is null, avoiding allocation on every capture when disabled. Tests: env var parsing, DSN-less init, pipeline bypass, Hub behavior, DSN-less DSC creation, and stream-attachment retention.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3a288c0. Configure here.
| // single-use stream attachments first so both serializations read independent copies. | ||
| BufferSingleUseAttachmentsForSpotlight(hint); | ||
| SendToSpotlight(Envelope.FromEvent(@event, _options.DiagnosticLogger, hint.Attachments.ToList())); | ||
| } |
There was a problem hiding this comment.
Spotlight events omit session updates
Medium Severity
SendToSpotlight runs before session side effects that set scope.SessionUpdate for crash and error reporting. The Spotlight envelope is built without that session item, while the main pipeline envelope includes it, so Spotlight sees a different payload for the same capture.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 3a288c0. Configure here.
| { | ||
| SendToSpotlight(envelope, ownsEnvelope: false); | ||
| } | ||
|
|
There was a problem hiding this comment.
Feedback double-serialize breaks attachments
Medium Severity
Feedback calls SendToSpotlight on the same envelope later passed to CaptureEnvelope, without the stream-attachment buffering used in DoSendEvent. A second serialization can consume single-use attachment streams so the main feedback envelope loses attachment data.
Reviewed by Cursor Bugbot for commit 3a288c0. Configure here.
| { | ||
| // Keep the original attachment so the main pipeline is unaffected. | ||
| _options.LogError(e, "Failed to buffer attachment '{0}' for Spotlight.", attachment.FileName); | ||
| buffered.Add(attachment); |
There was a problem hiding this comment.
Buffer failure still sends Spotlight
Medium Severity
When buffering a non-byte/file attachment fails, the catch block keeps the original attachment and SendToSpotlight still runs. Spotlight’s serialization reads the stream first and can leave the main pipeline envelope without usable attachment content.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 3a288c0. Configure here.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
CaptureEnvelope skips Spotlight entirely
Medium Severity
Spotlight is only invoked from explicit capture paths for events, transactions, and feedback. CaptureEnvelope no longer forwards to Spotlight, so sessions, check-ins, logs, metrics, and other worker envelopes never reach the sidecar after removing the transport wrapper.
Reviewed by Cursor Bugbot for commit 3a288c0. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| // pipeline will still drop this transaction. | ||
| var spotlightTx = new TransactionTracer(this, context) | ||
| { | ||
| IsSampled = false, | ||
| SampleRate = sampleRate, | ||
| SampleRand = sampleRand, | ||
| DynamicSamplingContext = dynamicSamplingContext | ||
| }; | ||
| spotlightTx.DynamicSamplingContext ??= spotlightTx.CreateDynamicSamplingContext(_options, _replaySession); | ||
| return spotlightTx; | ||
| } |
There was a problem hiding this comment.
Bug: When Spotlight is enabled, transactions sampled out due to backpressure are incorrectly reported with a discard reason of SampleRate instead of Backpressure.
Severity: MEDIUM
Suggested Fix
The discardReason computed in Hub.StartTransaction should be preserved and passed along with the transaction. This could involve adding a DiscardReason property to TransactionTracer and setting it when the tracer is created. SentryClient.CaptureTransaction should then use this preserved reason instead of hardcoding SampleRate.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/Sentry/Internal/Hub.cs#L251-L266
Potential issue: When Spotlight is enabled, `Hub.StartTransaction` creates a
`TransactionTracer` for transactions that are sampled out, so that span data can be
recorded for Spotlight. However, if a transaction is sampled out due to backpressure,
the computed `discardReason` of `Backpressure` is not stored on this
`TransactionTracer`. Consequently, when `SentryClient.CaptureTransaction` later
processes this transaction, it defaults to reporting the discard reason as `SampleRate`.
This leads to inaccurate client-side metrics, preventing users from correctly
identifying when transactions are dropped due to backpressure versus regular sampling.
Also affects:
src/Sentry/SentryClient.cs:212~216
Did we get this right? 👍 / 👎 to inform future reviews.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5302 +/- ##
==========================================
- Coverage 74.18% 73.64% -0.55%
==========================================
Files 508 495 -13
Lines 18353 18071 -282
Branches 3586 3529 -57
==========================================
- Hits 13615 13308 -307
- Misses 3866 3890 +24
- Partials 872 873 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jamescrosswell
left a comment
jamescrosswell
left a comment
There was a problem hiding this comment.
Thanks for the PR @mattico - it's a big one with lot's of potential gotchas and unintended consequences so I think it will need careful discussion, review and probably a few attempts to get right.
I left some comments. The main one is the question I raised about the high level approach:
- Should we try to tweak the SDK do that it can be initialised without a DSN when Spotlight is enabled? This has lots of domino effects (some of which I'm worried won't be caught by our existing test suite because we never anticipated this possibility).
- One alternative might be to have a dedicated 'Spotlight only' DSN that our regular transport recognised and simply discarded envelopes for... this might limit the change that needs to happen elsewhere in the codebase (where we'd previously been able to assume the presence of a DSN if the SDK was enabled).
I think the AI bots made some useful observations as well... some of them only relevant if we decide to do this by allowing the SDK to be initialised without a DSN but others would need addressing regardless.
|
|
||
| var propagationContext = CurrentScope.PropagationContext; | ||
| return propagationContext.GetOrCreateDynamicSamplingContext(_options, _replaySession).ToBaggageHeader(); | ||
| // GetOrCreateDynamicSamplingContext returns null when there is no DSN (e.g. Spotlight-only mode); |
There was a problem hiding this comment.
This implies trace propagation will be broken. That may be unavoidable but worth noting that there will be capability degradation in the SDK when using spotlight only - Sentry's distributed tracing won't work.
| { | ||
| if (_options.EnableSpotlight) | ||
| { | ||
| // When Spotlight is enabled, use a full TransactionTracer even for sampled-out transactions |
There was a problem hiding this comment.
Is this per some documentation on how this should be implemented or an arbitraty design decision? It definitely adds complexity since whether a full transaction tracer is present is now determined by an extra variable (that we may or may not have test cases for in other unrelated logic in the solution).
This additional complexity also seems like throw away code since the plan is to phase out transactions entirely and replace them with a streaming 'span only' performance tracing solution.
I'd be reluctant to pour time into trying to get this right (and fix any follow up bugs we don't catch in the initial review) if this is the case.
| _options.LogDebug("No cache directory path specified. Skipping caching transport creation."); | ||
| // No DSN — use a no-op transport (e.g. Spotlight-only mode). | ||
| _options.LogDebug("No DSN configured. Using no-op transport for Sentry."); | ||
| transport = NoOpTransport.Instance; |
There was a problem hiding this comment.
This seems like the guts of the solution - it's an interesting solution to the problem if we can work around some of the other side effects of not having a DSN (like distributed tracing not working).
Possibly an alternative solution would be to have some 'reserved' DSN that our regular transport basically treated as the equivalent of dev/null... that way we would have a valid DSN that could be used for things like the creation of a DynamicSamplingContext. I'm not sure if that's a better (or even good) idea - but maybe worth considering.
| options.JsonPreserveReferences = JsonPreserveReferences ?? options.JsonPreserveReferences; | ||
| options.EnableSpotlight = EnableSpotlight ?? options.EnableSpotlight; | ||
| options.SpotlightUrl = SpotlightUrl ?? options.SpotlightUrl; | ||
| // Deliberately not the usual `?? options.X` pattern: only assign when bound, to avoid marking |
There was a problem hiding this comment.
Won't this be inconsistent with the way other settings get bound/resolved? What's the reason for this?
2 participants
A follow-up to #5025, implements the remaining parts of #3481. Endeavors to implement the remaining behaviors mentioned in the Spotlight integration spec: functioning in DSN-less mode when Spotlight is enabled, having Spotlight envelopes bypass the processing pipeline, and the specification for env/config behavior.
As mentioned previously the Spotlight integration spec for the config is a bit at odds with the existing SentryOptions interface - this could be cleaned up at the next semver break but for now it maintains API compatibility at the cost of slight mismatch between interface and semantics.
I tried to make the pipeline changes as un-invasive as possible but a fair amount of refactoring was required because the integration point for Spotlight moved much earlier in order to implement the processing-bypass behavior.
There is some tricky stuff related to envelope serialization that I tried to test thoroughly but could take a careful review.
I probably won't have time to iterate on this PR for several weeks but I didn't want this code to languish on my hard drive any longer.