-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat: Add spotlightBrowser integration #13263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
1b83a36
81c7ba2
0deb983
2954e41
fdf4eac
f88efc5
5965324
2097073
f31f7d8
074dccd
cf26dad
50fcda5
ff233c7
f89533f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,2 @@ | ||
export { debugIntegration } from '@sentry/core'; | ||
export { spotlightBrowserIntegration } from '../integrations/spotlight'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,91 @@ | ||
import { getNativeImplementation } from '@sentry-internal/browser-utils'; | ||
import { defineIntegration } from '@sentry/core'; | ||
import type { Client, Envelope, Event, IntegrationFn } from '@sentry/types'; | ||
import { logger, serializeEnvelope } from '@sentry/utils'; | ||
import type { WINDOW } from '../helpers'; | ||
|
||
import { DEBUG_BUILD } from '../debug-build'; | ||
|
||
export type SpotlightConnectionOptions = { | ||
/** | ||
* Set this if the Spotlight Sidecar is not running on localhost:8969 | ||
* By default, the Url is set to http://localhost:8969/stream | ||
*/ | ||
sidecarUrl?: string; | ||
}; | ||
|
||
export const INTEGRATION_NAME = 'SpotlightBrowser'; | ||
|
||
const _spotlightIntegration = ((options: Partial<SpotlightConnectionOptions> = {}) => { | ||
const sidecarUrl = options.sidecarUrl || 'http://localhost:8969/stream'; | ||
|
||
return { | ||
name: INTEGRATION_NAME, | ||
setup: () => { | ||
DEBUG_BUILD && logger.log('Using Sidecar URL', sidecarUrl); | ||
}, | ||
// We don't want to send interaction transactions/root spans created from | ||
// clicks within Spotlight to Sentry. Neither do we want them to be sent to | ||
// spotlight. | ||
processEvent: event => (isSpotlightInteraction(event) ? null : event), | ||
afterAllSetup: (client: Client) => { | ||
setupSidecarForwarding(client, sidecarUrl); | ||
}, | ||
}; | ||
}) satisfies IntegrationFn; | ||
|
||
function setupSidecarForwarding(client: Client, sidecarUrl: string): void { | ||
const makeFetch: typeof WINDOW.fetch | undefined = getNativeImplementation('fetch'); | ||
let failCount = 0; | ||
|
||
client.on('beforeEnvelope', (envelope: Envelope) => { | ||
if (failCount > 3) { | ||
logger.warn('[Spotlight] Disabled Sentry -> Spotlight integration due to too many failed requests:', failCount); | ||
return; | ||
} | ||
|
||
makeFetch(sidecarUrl, { | ||
method: 'POST', | ||
body: serializeEnvelope(envelope), | ||
headers: { | ||
'Content-Type': 'application/x-sentry-envelope', | ||
}, | ||
mode: 'cors', | ||
}).then( | ||
res => { | ||
if (res.status >= 200 && res.status < 400) { | ||
// Reset failed requests counter on success | ||
failCount = 0; | ||
} | ||
}, | ||
err => { | ||
failCount++; | ||
logger.error( | ||
"Sentry SDK can't connect to Sidecar is it running? See: https://spotlightjs.com/sidecar/npx/", | ||
err, | ||
); | ||
}, | ||
); | ||
}); | ||
} | ||
|
||
/** | ||
* Use this integration to send errors and transactions to Spotlight. | ||
* | ||
* Learn more about spotlight at https://spotlightjs.com | ||
*/ | ||
export const spotlightBrowserIntegration = defineIntegration(_spotlightIntegration); | ||
|
||
/** | ||
* Flags if the event is a transaction created from an interaction with the spotlight UI. | ||
*/ | ||
export function isSpotlightInteraction(event: Event): boolean { | ||
return Boolean( | ||
event.type === 'transaction' && | ||
event.spans && | ||
event.contexts && | ||
event.contexts.trace && | ||
event.contexts.trace.op === 'ui.action.click' && | ||
event.spans.some(({ description }) => description && description.includes('#sentry-spotlight')), | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -311,7 +311,15 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> { | |
|
||
/** @inheritdoc */ | ||
public init(): void { | ||
if (this._isEnabled()) { | ||
if ( | ||
this._isEnabled() || | ||
// Force integrations to be setup even if no DSN was set when we have | ||
// Spotlight enabled. This is particularly important for browser as we | ||
// don't support the `spotlight` option there and rely on the users | ||
// adding the `spotlightBrowserIntegration()` to their integrations which | ||
// wouldn't get initialized with the check below when there's no DSN set. | ||
this._options.integrations.some(({ name }) => name.startsWith('Spotlight')) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is fine given spotlight is pretty much a one-off at the moment. Another option would be to adjust our integrations API to acomodate for such scenarios, say in an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. I thought about adding a special property like |
||
) { | ||
this._setupIntegrations(); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -17,7 +17,7 @@ import { | |||||
setOpenTelemetryContextAsyncContextStrategy, | ||||||
setupEventContextTrace, | ||||||
} from '@sentry/opentelemetry'; | ||||||
import type { Client, Integration, Options } from '@sentry/types'; | ||||||
import type { Integration, Options } from '@sentry/types'; | ||||||
import { | ||||||
consoleSandbox, | ||||||
dropUndefinedKeys, | ||||||
|
@@ -36,7 +36,7 @@ import { modulesIntegration } from '../integrations/modules'; | |||||
import { nativeNodeFetchIntegration } from '../integrations/node-fetch'; | ||||||
import { onUncaughtExceptionIntegration } from '../integrations/onuncaughtexception'; | ||||||
import { onUnhandledRejectionIntegration } from '../integrations/onunhandledrejection'; | ||||||
import { spotlightIntegration } from '../integrations/spotlight'; | ||||||
import { INTEGRATION_NAME as SPOTLIGHT_INTEGRATION_NAME, spotlightIntegration } from '../integrations/spotlight'; | ||||||
import { getAutoPerformanceIntegrations } from '../integrations/tracing'; | ||||||
import { makeNodeTransport } from '../transports'; | ||||||
import type { NodeClientOptions, NodeOptions } from '../types'; | ||||||
|
@@ -140,13 +140,19 @@ function _init( | |||||
const scope = getCurrentScope(); | ||||||
scope.update(options.initialScope); | ||||||
|
||||||
if (options.spotlight && !options.integrations.some(({ name }) => name === SPOTLIGHT_INTEGRATION_NAME)) { | ||||||
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can also do
Suggested change
But I think the current version expresses the intent a bit more clearly: if there are no integrations matching this name, add it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm fine with any of these, as long as we only add the integration if it isn't yet present 👍 |
||||||
options.integrations.push( | ||||||
spotlightIntegration({ | ||||||
sidecarUrl: typeof options.spotlight === 'string' ? options.spotlight : undefined, | ||||||
}), | ||||||
); | ||||||
} | ||||||
|
||||||
const client = new NodeClient(options); | ||||||
// The client is on the current scope, from where it generally is inherited | ||||||
getCurrentScope().setClient(client); | ||||||
|
||||||
if (isEnabled(client)) { | ||||||
client.init(); | ||||||
} | ||||||
client.init(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was a bit scared when I saw this but as far as I can tell we don't extend the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Event if we extend, aren't we supposed to call |
||||||
|
||||||
logger.log(`Running in ${isCjs() ? 'CommonJS' : 'ESM'} mode.`); | ||||||
|
||||||
|
@@ -158,20 +164,6 @@ function _init( | |||||
|
||||||
updateScopeFromEnvVariables(); | ||||||
|
||||||
if (options.spotlight) { | ||||||
// force integrations to be setup even if no DSN was set | ||||||
// If they have already been added before, they will be ignored anyhow | ||||||
const integrations = client.getOptions().integrations; | ||||||
for (const integration of integrations) { | ||||||
client.addIntegration(integration); | ||||||
} | ||||||
client.addIntegration( | ||||||
spotlightIntegration({ | ||||||
sidecarUrl: typeof options.spotlight === 'string' ? options.spotlight : undefined, | ||||||
}), | ||||||
); | ||||||
} | ||||||
|
||||||
// If users opt-out of this, they _have_ to set up OpenTelemetry themselves | ||||||
// There is no way to use this SDK without OpenTelemetry! | ||||||
if (!options.skipOpenTelemetrySetup) { | ||||||
|
@@ -336,7 +328,3 @@ function startSessionTracking(): void { | |||||
} | ||||||
}); | ||||||
} | ||||||
|
||||||
function isEnabled(client: Client): boolean { | ||||||
return client.getOptions().enabled !== false && client.getTransport() !== undefined; | ||||||
} |
Uh oh!
There was an error while loading. Please reload this page.