feat(node): Add Spotlight option to Node SDK #9629
Conversation
|
Just FYI weve agreed upon this interface for Spotlight through all SDKs. If anyone thinks we're missing anything please chime in, but we are intentionally not going to use the integrations interface as its error-prone for customers. |
There was a problem hiding this comment.
UPDATE: This comment is outdated, it seems like we need the top level option after all.
I still think this should be an integration for now rather than immediately a top level option. Reasons:
- A lot of SDKs inherit from Node (e.g. serverless) and we can't guarantee that this works (or doesn't at least cause problems) everywhere but the option will be there.
- fetch only works natively in Node 18+, so the current implementation will not work in our older supported Node versions
- Instead of jumping through the hoops of getting this bullet proof within the Node SDK right now, let's start with the integration where we can more easily say "Only use this with Node 18+ and in Dev mode". We can still make this a top-level option later on.
Maybe I'm overthinking but I wanna avoid adding something rushed to the SDK (or other non-JS SDKs for that matter) that can very well stand on its own.
Or IOW, does
Sentry.init({
spotlight: true
})
// vs
Sentry.init({
integrations: [new Spotlight()]
})really justify this effort?
| * @returns | ||
| */ | ||
| export class Spotlight implements Integration { | ||
| public name = 'Spotlight'; |
There was a problem hiding this comment.
we should set static id as well, shouldn't we?
size-limit report 📦
|
| res => { | ||
| res.on('data', () => { | ||
| // Drain socket | ||
| }); | ||
|
|
||
| res.on('end', () => { | ||
| // Drain socket | ||
| }); | ||
| res.setEncoding('utf8'); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
tbh, I'm not sure if we need this but I found the same logic in the Http transport request creation function.
HazAT
changed the title
|
|
||
| if (options.spotlight) { | ||
| const client = getCurrentHub().getClient(); | ||
| if (client && client.addIntegration) { |
There was a problem hiding this comment.
Can we check if this has run, and only then call this? Even if we check this inside of setupIntegrations(), may be a bit cleaner I'd say?
There was a problem hiding this comment.
Discussed this offline quickly: Checking this is not easy outside the BaseClient class. Given this call no-ops if integrations were already set up, let's keep it as is for now.
Lms24
changed the title
| const req = http.request( | ||
| { | ||
| method: 'POST', | ||
| path: '/stream', |
There was a problem hiding this comment.
Looks like spotlightUrl.path should be /stream already? Is hardcoding intentional?
There was a problem hiding this comment.
If I'm not mistaken (it's laaate here 😅) spotlightUrl is the optionally user configurable part of the URL - protocol, host and port. The path I think we're going to mandate to be /stream.
There was a problem hiding this comment.
Id just make it the full URL tbqh. That makes it easier for us to swap out different proxies later if we wanted.
e.g. make it http://localhost:8969/stream as the default (or w/e it is)
This PR adds a new top level option called
spotlightto Node init options. Under the hood, if this option is true,Spotlightintegration is added. This integration will make ahttppost request to the sidecar URL. Either we take the default sidecar URL or users provide their own URL:This option should also work in Node Experimental, given that Node experimental just calls the node init function.