feat: Add Spotlight integration for local development debugging - #1085
feat: Add Spotlight integration for local development debugging#1085DAcodedBEAT wants to merge 6 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1085 +/- ##
==========================================
- Coverage 86.04% 81.90% -4.14%
==========================================
Files 62 75 +13
Lines 6090 7543 +1453
==========================================
+ Hits 5240 6178 +938
- Misses 635 1087 +452
- Partials 215 278 +63 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for your PR, but we won't merge this anytime soon. We have a big refactor coming in our transport and we can look at this afterwards maybe. |
|
@cleptric Thanks for the update! I first asked about Spotlight support back in June 2024 and only heard back a year later with "no progress", so I went ahead and built this to give the community something useful and closer to what Python/JS already have (and to help offload some effort since the Sentry team is swamped). I understand holding off until the transport refactor, but is there a public roadmap or place where this is being discussed? It would really help contributors know what’s changing and avoid working on features that might get blocked. Happy to rebase once things stabilize! |
493d2c2 to
7ba9727
Compare
|
I'm ok merging this now instead of waiting for our refactoring, as the surface area is indeed rather small. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
7ba9727 to
ae10b0d
Compare
Sorry for missing the lint errors! I've fixed it and re-pushed (trying to keep a clean git history for y'all). Let me know if there's anything else I can do to get this merged! |
giortzisg
left a comment
giortzisg
left a comment
There was a problem hiding this comment.
Thanks for the contribution. Left some small comments. Let's get them fixed and we can merge this.
a0b6588 to
d660c5e
Compare
d660c5e to
dd1af08
Compare
|
@giortzisg / @cleptric - may I please get some re-reviews on this? |
|
Sorry for the radio silence @DAcodedBEAT -- I'll get to this this week. We've also created a spec for how Spotlight integration should behave in all SDKs: https://develop.sentry.dev/sdk/expected-features/spotlight I'll be making suggestions based on this (or you can proactively adjust your patch accordingly) |
giortzisg
left a comment
giortzisg
left a comment
There was a problem hiding this comment.
We should update the implementation to also use the internal/telemetry/scheduler that we added for the new transport (we need to support both for now unfortunately) and also make the spotlight transport implementation async. Also left some comments for inconsistencies with the spec.
|
|
||
| // Check for Spotlight environment variable | ||
| if !options.Spotlight { | ||
| if spotlightEnv := os.Getenv("SENTRY_SPOTLIGHT"); spotlightEnv == "true" || spotlightEnv == "1" { |
There was a problem hiding this comment.
We should adhere to these values.
We also need to extract the url if provided with SENTRY_SPOTLIGHT
| // Always send to Spotlight | ||
| st.sendToSpotlight(event) |
There was a problem hiding this comment.
The send to spotlight call is sync here, so it kinda defeats the purpose of the async transport. I think the smart solution here to not couple the spotlight transport with the background worker is to inverse the dependency and modify the Sync and Async transport implementations to just invoke the spotlight transport when it exists. This makes the change to the new transport also minimal, where we would just invoke spotlight.Send on the scheduler, along the http request to the sentry backend.
| } | ||
|
|
||
| func (st *SpotlightTransport) sendToSpotlight(event *Event) { | ||
| ctx := context.Background() |
There was a problem hiding this comment.
we should also support context cancellation during shutdown. Currently during shutdown spotlight might block and we won't get a graceful shutdown.
| client: &http.Client{ | ||
| Timeout: 5 * time.Second, | ||
| }, |
There was a problem hiding this comment.
the client should respect the proxy settings. why aren't we using the underlying client here?
d01d90c to
bb13c24
Compare
Semver Impact of This PR🟡 Minor (new features) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨
Bug Fixes 🐛
Internal Changes 🔧Ai
Other
🤖 This preview updates automatically when you update the PR. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| // For Spotlight: build and send envelope for enhanced data collection | ||
| if client.options.Spotlight { | ||
| envelope := client.buildEnvelopeFromEvent(event) | ||
| client.Transport.SendEnvelope(envelope) | ||
| } | ||
| // Default path: send event directly for backwards compatibility | ||
| client.Transport.SendEvent(event) |
There was a problem hiding this comment.
Bug: When Spotlight is enabled, processEvent calls both SendEnvelope and SendEvent on the transport, causing each event to be sent to Sentry twice.
Severity: HIGH
Suggested Fix
In processEvent, modify the logic to only call client.Transport.SendEnvelope(envelope) when client.options.Spotlight is true. The SendEnvelope method on the transport should be solely responsible for forwarding to both Spotlight and Sentry. The redundant call to client.Transport.SendEvent(event) should be removed.
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: client.go#L989-L995
Potential issue: When Spotlight is enabled and `telemetryProcessor` is `nil`, the
`processEvent` function calls both `client.Transport.SendEnvelope()` and
`client.Transport.SendEvent()`. The `client.Transport` is a `SpotlightTransport` which
wraps an underlying transport like `HTTPTransport`. The
`SpotlightTransport.SendEnvelope` call results in the underlying transport sending the
event to Sentry. The subsequent `SpotlightTransport.SendEvent` call sends the same event
to Sentry again. This results in every captured event being sent to Sentry twice.
Did we get this right? 👍 / 👎 to inform future reviews.
| FlushWithContext(ctx context.Context) bool | ||
| Configure(options ClientOptions) | ||
| SendEvent(event *Event) | ||
| SendEnvelope(envelope *protocol.Envelope) |
There was a problem hiding this comment.
We should not implement sendEnvelope for the HttpTransport
| // SendEnvelope sends an envelope to the Sentry server. | ||
| // Currently converts envelope back to event for compatibility. | ||
| // In the future, this will be the primary method. | ||
| func (t *HTTPTransport) SendEnvelope(envelope *protocol.Envelope) { |
| t.SendEventWithContext(context.Background(), event) | ||
| } | ||
|
|
||
| func (t *HTTPSyncTransport) SendEnvelope(envelope *protocol.Envelope) { |
| wg sync.WaitGroup | ||
| } | ||
|
|
||
| func NewSpotlightTransport(underlying Transport) *SpotlightTransport { |
There was a problem hiding this comment.
following from the previous comment, when we setup the transport on the client, we already convert the internal/http/transport to a Transport interface with the internalAsyncTransportAdapter so there is no point in satisfying the SendEnvelope on the scope of this PR.
|
|
||
| // buildEnvelopeFromEvent builds an envelope from an event. | ||
| // This is used to send events via the envelope-based transport API. | ||
| func (client *Client) buildEnvelopeFromEvent(event *Event) *protocol.Envelope { |
There was a problem hiding this comment.
this is also not needed, the adapter should suffice.
| // For Spotlight: build and send envelope for enhanced data collection | ||
| if client.options.Spotlight { | ||
| envelope := client.buildEnvelopeFromEvent(event) | ||
| client.Transport.SendEnvelope(envelope) | ||
| } | ||
| // Default path: send event directly for backwards compatibility |
There was a problem hiding this comment.
the transport is already wrapped with spotlight, so that part is not needed. the bugbot comments are valid.
| // Flush waits for the underlying transport to send pending events to Sentry. | ||
| // In-flight Spotlight sends are best-effort and not waited on. | ||
| func (st *SpotlightTransport) Flush(timeout time.Duration) bool { | ||
| return st.underlying.Flush(timeout) | ||
| } | ||
|
|
||
| func (st *SpotlightTransport) FlushWithContext(ctx context.Context) bool { | ||
| return st.underlying.FlushWithContext(ctx) | ||
| } |
There was a problem hiding this comment.
this probably doesn't work with an empty dsn. the noop transport returns from flush immediately so no events would be delivered, we also need to wait for the spotlight events.
| if options.Spotlight { | ||
| if options.SampleRate != 1.0 { | ||
| debuglog.Printf("Overriding SampleRate from %.2f to 1.0 for Spotlight", options.SampleRate) | ||
| options.SampleRate = 1.0 | ||
| } | ||
| if !options.SendDefaultPII { | ||
| debuglog.Println("Enabling SendDefaultPII for Spotlight") | ||
| options.SendDefaultPII = true | ||
| } | ||
| } |
There was a problem hiding this comment.
We shouldn't override it globally, because this would affect the sample rate and pii of the default sentry client
|
Hey @DAcodedBEAT any updates on this? |
bb13c24 to
9b748d7
Compare
Adds support for Sentry Spotlight (https://spotlightjs.com/), a local dev-time debugging sidecar. When enabled via ClientOptions.Spotlight or the SENTRY_SPOTLIGHT env var, events are also forwarded to a local Spotlight instance (default http://localhost:8969/stream). - Uses the same transport path as real Sentry delivery by default. - Never affects what's sent upstream: sample rate/PII overrides only apply with no DSN configured. - Sidecar connectivity failures are backed off and logged independently.
9b748d7 to
d226ed0
Compare
- Envelope clone still raced with the transport: I'd made the clone itself synchronous earlier, but sendItem called transport.SendEnvelope (which can mutate the envelope on a background worker) before spotlight.Send read it. Swapped the order so Spotlight clones first. - TracesSampler overrides TracesSampleRate, so forcing SampleRate to 1.0 for Spotlight didn't help if a custom sampler was also set - it could still drop transactions. Now cleared it too.
Missed this when I fixed SpotlightTransport earlier. Send runs on the scheduler's background goroutine and can race a concurrent Flush/Close call from another goroutine - the pending counter only covers items already buffered before Flush was called, not a brand new Send. This one's actually worse than SpotlightTransport since it backs the default Spotlight path everyone uses. Switched to the same atomic counter + WaitForZero pattern, plus a concurrent stress test under -race.
- Spotlight was silently skipped whenever the real Sentry transport was rate-limited or out of capacity, since processItems returned before ever reaching sendItem. Spotlight is a separate destination and should still get a copy in that case - otherwise local debugging misses exactly the events that got dropped on the way to Sentry. processItems now only gates the real transport send on rate-limit/ capacity, not the Spotlight forward. - SpotlightTransport.SendEvent handed the event pointer to a background goroutine that read it later (serializing to JSON), racing with anything mutating the event's nested maps after SendEvent returned. Moved the serialization/envelope-building synchronous, before the goroutine spawns - only the network POST runs async now, matching what spotlightEnvelopeSender already does.
Two more instances of the counter-race pattern bots kept finding: - Scheduler.Add incremented pending after Offer already made the item visible to run(), leaving a narrow window where a concurrent Flush could drain and decrement it before the increment landed. Increment now happens before Offer, rolled back if the item is rejected. - The dropped-item callback decremented pending for every overflow reason, but "buffer_full_drop_newest"/"unknown_overflow_policy" reject the incoming item itself, which Add never incremented pending for - only "buffer_full_drop_oldest[_bucket]" evicts an already-counted item. Decrementing for the others drove pending permanently negative, so every Flush after that would spin to its deadline and report failure. Not reachable with the DropOldest policy this SDK actually configures, but real for any future policy change, so it's regression-tested directly against a DropNewest buffer.
- WaitForZero could report a timeout even when the counter had just hit zero, since select picks randomly between ready channels. Now double-checks the counter before returning false. - Flush only force-drained buffers once, so anything captured right after (a log, say) sat waiting on its own batch timeout instead of being picked up, and Flush would spin to its deadline and fail. Not Spotlight-specific - this hit the plain flush path too. Now loops drain+wait until pending settles or ctx ends. Two other bot comments on this diff were stale re-reports against an earlier commit, already fixed since.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7c2d623. Configure here.
| spotlight = newSpotlightEnvelopeSender(client.options) | ||
| } | ||
|
|
||
| client.telemetryProcessor = telemetry.NewProcessor(buffers, transport, client.dsn, client.sdkInfo, client.reportRecorder, spotlight) |
There was a problem hiding this comment.
Misleading logs in Spotlight-only mode
Medium Severity
The default Spotlight-only path (empty Dsn) wires the telemetry processor to internal/http.NoopTransport, which logs that the DSN is invalid and that every envelope is dropped. With Debug: true as in the example, that implies events are lost even though Spotlight still receives them. The legacy noopTransport.Configure message was updated for this case, but the default path was not.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 7c2d623. Configure here.
4 participants
resolves: #846
Add SpotlightTransport that sends events to local Spotlight server for real-time debugging during development. The integration works by wrapping the existing transport and duplicating events to Spotlight.
Features: