feat: Add Spotlight integration for local development debugging#1085
feat: Add Spotlight integration for local development debugging#1085DAcodedBEAT wants to merge 12 commits intogetsentry:masterfrom
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! |
DAcodedBEAT
force-pushed
the
feature/spotlight
branch
from
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.
DAcodedBEAT
force-pushed
the
feature/spotlight
branch
from
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.
DAcodedBEAT
force-pushed
the
feature/spotlight
branch
2 times, most recently
from
a0b6588 to
d660c5e
Compare
DAcodedBEAT
force-pushed
the
feature/spotlight
branch
from
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.
client.go
Outdated
|
|
||
| // 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
| if opts.Spotlight { | ||
| transport = NewSpotlightTransport(transport) | ||
| } |
There was a problem hiding this comment.
we should also override the sample rate to be 1.0 and PII to true when spotlight is enabled
transport.go
Outdated
| // 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.
transport.go
Outdated
| } | ||
|
|
||
| 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?
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: - Spotlight transport decorator supporting custom URL overrides - SENTRY_SPOTLIGHT environment variable for easy enabling - Example program demonstrating usage patterns - README documentation update
Address review comments from PR: - Move SENTRY_SPOTLIGHT env var check to NewClient - Remove noopTransport check in SendEvent - Use event.Sdk.Name/Version for User-Agent header - Use context for HTTP requests - Always drain HTTP response bodies - Use shared MockTransport from mocks.go
Explicitly handle error from NewDsn to prevent nil pointer panic. Sentry delivery is unaffected as it occurs before Spotlight.
Add FlushCount() and CloseCount() accessors to MockTransport with optional ResetCounts() for multi-assertion tests. Update tests to verify the underlying transport methods are properly delegated instead of silently broken.
…actor) Add envelope-level transport method to support spec-compliant Spotlight integration with full PII collection and 100% sample rate. Phase 1 changes: - Add SendEnvelope() method to Transport interface - Implement SendEnvelope() in all transport types (HTTP, HTTPSync, noop, etc) - Add temporary envelope-to-event conversion for backwards compatibility - Enhance MockTransport with SendEnvelope() and tracking counters - Add complete SENTRY_SPOTLIGHT env var parsing (all truthy/falsy values) - Implement precedence rules for config vs env var - Add debug logging for config conflicts Backwards compatible: SendEvent() still works, SendEnvelope() delegates to it Next phases: Move envelope building to Client, implement Spotlight cloning
Move envelope building to Client and refactor SpotlightTransport to work at envelope level, enabling proper spec compliance with full PII and 100% sample rate for Spotlight while maintaining backwards compatibility. Phase 2 changes: - Add buildEnvelopeFromEvent() helper in Client to build envelopes - Update processEvent() to build and send envelopes via SendEnvelope() - Maintain backwards compatibility by also calling SendEvent() - Implement SpotlightTransport.SendEnvelope() with envelope cloning - Add cloneEnvelope() to create deep copies for Spotlight - Implement sendToSpotlightServer() using proper envelope serialization - Use HTTP client from SpotlightTransport for consistent timeout handling Next: Remove redundant SendEvent calls and optimize transport layer 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Only send envelopes when Spotlight is enabled to avoid data loss through serialization/deserialization during testing. Default path still uses SendEvent for backwards compatibility. When Spotlight is enabled: - Build envelope and send via SendEnvelope (for Spotlight processing) - Also send event via SendEvent (for Sentry ingestion) When Spotlight is disabled: - Send event via SendEvent only (backwards compatible) This maintains test compatibility while enabling envelope-level Spotlight interception for future spec compliance. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Verify all spec requirements are met: - Default URL (http://localhost:8969/stream) used when no URL specified - Config URL takes precedence over env var URL - Env var URL applied correctly in all scenarios - Spotlight disabled by default, enabled only when explicitly configured Co-Authored-By: Claude <noreply@anthropic.com>
Add comprehensive tests for: - parseSpotlightEnvVar: all truthy/falsy/URL parsing scenarios - cloneAndModifyEnvelopeForSpotlight: envelope deep copying - modifyEventItemForSpotlight: event JSON modification and error handling Tests ensure proper envelope handling, item type differentiation, and graceful error recovery for invalid payloads. Co-Authored-By: Claude <noreply@anthropic.com>
- Fix inconsistent default URL comment (http://localhost:8969/ → http://localhost:8969/stream) - Clarify sample rate behavior: sampling happens at client level, not transport - Explain that Spotlight receives all PII data unfiltered - Document Flush behavior with Spotlight (non-blocking, best-effort) These clarifications address common DX confusion points about the Spotlight integration and make the intended behavior more explicit. Co-Authored-By: Claude <noreply@anthropic.com>
- Add helpful error message when Spotlight server is unreachable - Include installation instructions in error logs (npm install -g @spotlightjs/spotlight) - Document SENTRY_SPOTLIGHT env var parsing with examples - Truthy values: 'true', 'y', 'yes', 'on', '1' - Falsy values: 'false', 'n', 'no', 'off', '0' - URLs: any other string - Add precedence documentation - Clarify Spotlight-only usage in example (DSN optional for dev debugging) - Improve example comments to explain when to use Spotlight vs Sentry These changes improve DX for users setting up Spotlight integration. Co-Authored-By: Claude <noreply@anthropic.com>
- Make SendEvent/SendEnvelope non-blocking (run in goroutines) - Force SampleRate=1.0 and SendDefaultPII=true when Spotlight is enabled - only lowercase for keyword matching
DAcodedBEAT
force-pushed
the
feature/spotlight
branch
from
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.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| envelope := client.buildEnvelopeFromEvent(event) | ||
| client.Transport.SendEnvelope(envelope) | ||
| } | ||
| // Default path: send event directly for backwards compatibility |
There was a problem hiding this comment.
Events sent twice to both Sentry and Spotlight
High Severity
When Spotlight is enabled, processEvent calls both SendEnvelope and SendEvent on client.Transport, which is a SpotlightTransport. Each of those methods forwards to the underlying transport AND sends to Spotlight. Since HTTPTransport.SendEnvelope internally deserializes and calls SendEvent, the event is delivered to Sentry twice and to Spotlight twice. The SendEnvelope call at line 992 and the SendEvent call at line 995 are redundant — only one path is needed.
Additional Locations (1)
| cloned.AddItem(&protocol.EnvelopeItem{ | ||
| Header: item.Header, | ||
| Payload: append([]byte(nil), item.Payload...), | ||
| }) |
There was a problem hiding this comment.
Shallow copy of item headers in envelope clone
Medium Severity
cloneEnvelopeForSpotlight claims to create a "deep copy" but shares item.Header pointers between the original and cloned envelope. Since the clone is sent in a background goroutine while the original envelope continues through the underlying transport, concurrent access to the shared EnvelopeItemHeader is a data race risk.
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: