Skip to content
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

feat: Add spotlightBrowser integration #13263

Merged
merged 14 commits into from
Aug 12, 2024
2 changes: 1 addition & 1 deletion .size-limit.js
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ module.exports = [
import: createImport('init'),
ignore: ['next/router', 'next/constants'],
gzip: true,
limit: '38 KB',
limit: '38.03 KB',
},
// SvelteKit SDK (ESM)
{
Expand Down
5 changes: 3 additions & 2 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,11 @@
],
"deno.enablePaths": ["packages/deno/test"],
"editor.codeActionsOnSave": {
"source.organizeImports.biome": "explicit",
"source.organizeImports.biome": "explicit"
},
"editor.defaultFormatter": "biomejs.biome",
"[typescript]": {
"editor.defaultFormatter": "biomejs.biome"
}
},
"cSpell.words": ["arrayify"]
}
1 change: 1 addition & 0 deletions packages/browser/src/integrations-bundle/index.debug.ts
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
export { debugIntegration } from '@sentry/core';
export { spotlightBrowserIntegration } from '../integrations/spotlight';
91 changes: 91 additions & 0 deletions packages/browser/src/integrations/spotlight.ts
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) => {
BYK marked this conversation as resolved.
Show resolved Hide resolved
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')),
);
}
10 changes: 9 additions & 1 deletion packages/core/src/baseclient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'))
Copy link
Member

Choose a reason for hiding this comment

The 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 afterSdkInit hook that's aways invoked, regardless if the SDK is enabled or not. Will bring this up with the team but let's not block the PR on this as it's easily changeable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I thought about adding a special property like forced or something like that on the integration but I'll leave that debate to you folks

) {
this._setupIntegrations();
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/integration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export type IntegrationIndex = {

/**
* Remove duplicates from the given array, preferring the last instance of any duplicate. Not guaranteed to
* preseve the order of integrations in the array.
* preserve the order of integrations in the array.
*
* @private
*/
Expand Down
6 changes: 5 additions & 1 deletion packages/node/src/integrations/spotlight.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ type SpotlightConnectionOptions = {
sidecarUrl?: string;
};

const INTEGRATION_NAME = 'Spotlight';
export const INTEGRATION_NAME = 'Spotlight';

const _spotlightIntegration = ((options: Partial<SpotlightConnectionOptions> = {}) => {
const _options = {
Expand Down Expand Up @@ -66,6 +66,10 @@ function connectToSpotlight(client: Client, options: Required<SpotlightConnectio
},
},
res => {
if (res.statusCode && res.statusCode >= 200 && res.statusCode < 400) {
// Reset failed requests counter on success
failedRequests = 0;
}
res.on('data', () => {
// Drain socket
});
Expand Down
34 changes: 11 additions & 23 deletions packages/node/src/sdk/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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';
Expand Down Expand Up @@ -140,13 +140,19 @@ function _init(
const scope = getCurrentScope();
scope.update(options.initialScope);

if (options.spotlight && !options.integrations.some(({ name }) => name === SPOTLIGHT_INTEGRATION_NAME)) {
Copy link
Member Author

@BYK BYK Aug 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also do

Suggested change
if (options.spotlight && !options.integrations.some(({ name }) => name === SPOTLIGHT_INTEGRATION_NAME)) {
if (options.spotlight && options.integrations.every(({ name }) => name !== SPOTLIGHT_INTEGRATION_NAME)) {

But I think the current version expresses the intent a bit more clearly: if there are no integrations matching this name, add it.

Copy link
Member

Choose a reason for hiding this comment

The 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 👍
Thanks for fixing!

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();
Copy link
Member

Choose a reason for hiding this comment

The 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 init from BaseClient in the derived client classes. Given we guard the base client init implementation this should be fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Event if we extend, aren't we supposed to call super()? I think that's the responsibility of downstream class implementors and not something we should be worried about here?


logger.log(`Running in ${isCjs() ? 'CommonJS' : 'ESM'} mode.`);

Expand All @@ -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) {
Expand Down Expand Up @@ -336,7 +328,3 @@ function startSessionTracking(): void {
}
});
}

function isEnabled(client: Client): boolean {
return client.getOptions().enabled !== false && client.getTransport() !== undefined;
}
Loading