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(integration): Ensure LinkedErrors integration runs before all event processors #8956

Merged
merged 4 commits into from
Sep 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
40 changes: 17 additions & 23 deletions packages/browser/src/integrations/linkederrors.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { Event, EventHint, EventProcessor, Hub, Integration } from '@sentry/types';
import type { Client, Event, EventHint, Integration } from '@sentry/types';
import { applyAggregateErrorsToEvent } from '@sentry/utils';

import { exceptionFromError } from '../eventbuilder';
Expand Down Expand Up @@ -42,31 +42,25 @@ export class LinkedErrors implements Integration {
this._limit = options.limit || DEFAULT_LIMIT;
}

/** @inheritdoc */
public setupOnce(): void {
// noop
}

/**
* @inheritDoc
*/
public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void {
addGlobalEventProcessor((event: Event, hint?: EventHint) => {
const hub = getCurrentHub();
const client = hub.getClient();
const self = hub.getIntegration(LinkedErrors);

if (!client || !self) {
return event;
}

const options = client.getOptions();
applyAggregateErrorsToEvent(
exceptionFromError,
options.stackParser,
options.maxValueLength,
self._key,
self._limit,
event,
hint,
);
public preprocessEvent(event: Event, hint: EventHint | undefined, client: Client): void {
const options = client.getOptions();

return event;
});
applyAggregateErrorsToEvent(
exceptionFromError,
options.stackParser,
options.maxValueLength,
this._key,
this._limit,
event,
hint,
);
}
}
26 changes: 22 additions & 4 deletions packages/core/src/baseclient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
*/
public setupIntegrations(): void {
if (this._isEnabled() && !this._integrationsInitialized) {
this._integrations = setupIntegrations(this._options.integrations);
this._integrations = setupIntegrations(this, this._options.integrations);
this._integrationsInitialized = true;
}
}
Expand Down Expand Up @@ -315,7 +315,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
* @inheritDoc
*/
public addIntegration(integration: Integration): void {
setupIntegration(integration, this._integrations);
setupIntegration(this, integration, this._integrations);
}

/**
Expand Down Expand Up @@ -376,16 +376,23 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
}

// Keep on() & emit() signatures in sync with types' client.ts interface
/* eslint-disable @typescript-eslint/unified-signatures */

/** @inheritdoc */
public on(hook: 'startTransaction' | 'finishTransaction', callback: (transaction: Transaction) => void): void;
public on(hook: 'startTransaction', callback: (transaction: Transaction) => void): void;

/** @inheritdoc */
public on(hook: 'finishTransaction', callback: (transaction: Transaction) => void): void;

/** @inheritdoc */
public on(hook: 'beforeEnvelope', callback: (envelope: Envelope) => void): void;

/** @inheritdoc */
public on(hook: 'beforeSendEvent', callback: (event: Event, hint?: EventHint) => void): void;

/** @inheritdoc */
public on(hook: 'preprocessEvent', callback: (event: Event, hint?: EventHint) => void): void;

/** @inheritdoc */
public on(
hook: 'afterSendEvent',
Expand All @@ -412,14 +419,20 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
}

/** @inheritdoc */
public emit(hook: 'startTransaction' | 'finishTransaction', transaction: Transaction): void;
public emit(hook: 'startTransaction', transaction: Transaction): void;

/** @inheritdoc */
public emit(hook: 'finishTransaction', transaction: Transaction): void;

/** @inheritdoc */
public emit(hook: 'beforeEnvelope', envelope: Envelope): void;

/** @inheritdoc */
public emit(hook: 'beforeSendEvent', event: Event, hint?: EventHint): void;

/** @inheritdoc */
public emit(hook: 'preprocessEvent', event: Event, hint?: EventHint): void;

/** @inheritdoc */
public emit(hook: 'afterSendEvent', event: Event, sendResponse: TransportMakeRequestResponse | void): void;

Expand All @@ -439,6 +452,8 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
}
}

/* eslint-enable @typescript-eslint/unified-signatures */

/** Updates existing session based on the provided event */
protected _updateSessionFromEvent(session: Session, event: Event): void {
let crashed = false;
Expand Down Expand Up @@ -527,6 +542,9 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
if (!hint.integrations && integrations.length > 0) {
hint.integrations = integrations;
}

this.emit('preprocessEvent', event, hint);

return prepareEvent(options, event, hint, scope).then(evt => {
if (evt === null) {
return evt;
Expand Down
16 changes: 11 additions & 5 deletions packages/core/src/integration.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { Integration, Options } from '@sentry/types';
import type { Client, Integration, Options } from '@sentry/types';
import { arrayify, logger } from '@sentry/utils';

import { getCurrentHub } from './hub';
Expand Down Expand Up @@ -84,28 +84,34 @@ export function getIntegrationsToSetup(options: Options): Integration[] {
* @param integrations array of integration instances
* @param withDefault should enable default integrations
*/
export function setupIntegrations(integrations: Integration[]): IntegrationIndex {
export function setupIntegrations(client: Client, integrations: Integration[]): IntegrationIndex {
const integrationIndex: IntegrationIndex = {};

integrations.forEach(integration => {
// guard against empty provided integrations
if (integration) {
setupIntegration(integration, integrationIndex);
setupIntegration(client, integration, integrationIndex);
}
});

return integrationIndex;
}

/** Setup a single integration. */
export function setupIntegration(integration: Integration, integrationIndex: IntegrationIndex): void {
export function setupIntegration(client: Client, integration: Integration, integrationIndex: IntegrationIndex): void {
integrationIndex[integration.name] = integration;

if (installedIntegrations.indexOf(integration.name) === -1) {
integration.setupOnce(addGlobalEventProcessor, getCurrentHub);
installedIntegrations.push(integration.name);
__DEBUG_BUILD__ && logger.log(`Integration installed: ${integration.name}`);
}

if (client.on && typeof integration.preprocessEvent === 'function') {
const callback = integration.preprocessEvent.bind(integration);
client.on('preprocessEvent', (event, hint) => callback(event, hint, client));
}

__DEBUG_BUILD__ && logger.log(`Integration installed: ${integration.name}`);
}

// Polyfill for Array.findIndex(), which is not supported in ES5
Expand Down
51 changes: 18 additions & 33 deletions packages/node/src/integrations/linkederrors.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
import type { Event, EventHint, EventProcessor, Hub, Integration } from '@sentry/types';
import type { Client, Event, EventHint, Integration } from '@sentry/types';
import { applyAggregateErrorsToEvent } from '@sentry/utils';

import { exceptionFromError } from '../eventbuilder';
import { ContextLines } from './contextlines';

const DEFAULT_KEY = 'cause';
const DEFAULT_LIMIT = 5;
Expand Down Expand Up @@ -37,39 +36,25 @@ export class LinkedErrors implements Integration {
this._limit = options.limit || DEFAULT_LIMIT;
}

/** @inheritdoc */
public setupOnce(): void {
// noop
}

/**
* @inheritDoc
*/
public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void {
addGlobalEventProcessor(async (event: Event, hint?: EventHint) => {
const hub = getCurrentHub();
const client = hub.getClient();
const self = hub.getIntegration(LinkedErrors);

if (!client || !self) {
return event;
}

const options = client.getOptions();

applyAggregateErrorsToEvent(
exceptionFromError,
options.stackParser,
options.maxValueLength,
self._key,
self._limit,
event,
hint,
);

// If the ContextLines integration is enabled, we add source code context to linked errors
// because we can't guarantee the order that integrations are run.
const contextLines = getCurrentHub().getIntegration(ContextLines);
if (contextLines) {
await contextLines.addSourceContext(event);
}

return event;
});
public preprocessEvent(event: Event, hint: EventHint | undefined, client: Client): void {
const options = client.getOptions();

applyAggregateErrorsToEvent(
exceptionFromError,
options.stackParser,
options.maxValueLength,
this._key,
this._limit,
event,
hint,
);
}
}
52 changes: 42 additions & 10 deletions packages/types/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,21 +165,38 @@ export interface Client<O extends ClientOptions = ClientOptions> {

// HOOKS
// TODO(v8): Make the hooks non-optional.
/* eslint-disable @typescript-eslint/unified-signatures */

/**
* Register a callback for transaction start and finish.
* Register a callback for transaction start.
* Receives the transaction as argument.
*/
on?(hook: 'startTransaction' | 'finishTransaction', callback: (transaction: Transaction) => void): void;
on?(hook: 'startTransaction', callback: (transaction: Transaction) => void): void;

/**
* Register a callback for transaction finish.
* Receives the transaction as argument.
*/
on?(hook: 'finishTransaction', callback: (transaction: Transaction) => void): void;

/**
* Register a callback for transaction start and finish.
*/
on?(hook: 'beforeEnvelope', callback: (envelope: Envelope) => void): void;

/**
* Register a callback for before an event is sent.
* Register a callback for before sending an event.
* This is called right before an event is sent and should not be used to mutate the event.
* Receives an Event & EventHint as arguments.
*/
on?(hook: 'beforeSendEvent', callback: (event: Event, hint?: EventHint | void) => void): void;
on?(hook: 'beforeSendEvent', callback: (event: Event, hint?: EventHint | undefined) => void): void;

/**
* Register a callback for preprocessing an event,
* before it is passed to (global) event processors.
* Receives an Event & EventHint as arguments.
*/
on?(hook: 'preprocessEvent', callback: (event: Event, hint?: EventHint | undefined) => void): void;

/**
* Register a callback for when an event has been sent.
Expand All @@ -206,23 +223,36 @@ export interface Client<O extends ClientOptions = ClientOptions> {
on?(hook: 'otelSpanEnd', callback: (otelSpan: unknown, mutableOptions: { drop: boolean }) => void): void;

/**
* Fire a hook event for transaction start and finish. Expects to be given a transaction as the
* second argument.
* Fire a hook event for transaction start.
* Expects to be given a transaction as the second argument.
*/
emit?(hook: 'startTransaction' | 'finishTransaction', transaction: Transaction): void;
emit?(hook: 'startTransaction', transaction: Transaction): void;

/**
* Fire a hook event for transaction finish.
* Expects to be given a transaction as the second argument.
*/
emit?(hook: 'finishTransaction', transaction: Transaction): void;

/*
* Fire a hook event for envelope creation and sending. Expects to be given an envelope as the
* second argument.
*/
emit?(hook: 'beforeEnvelope', envelope: Envelope): void;

/*
* Fire a hook event before sending an event. Expects to be given an Event & EventHint as the
* second/third argument.
/**
* Fire a hook event before sending an event.
* This is called right before an event is sent and should not be used to mutate the event.
* Expects to be given an Event & EventHint as the second/third argument.
*/
emit?(hook: 'beforeSendEvent', event: Event, hint?: EventHint): void;
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this two separate hooks? If necessary by ignoring the lint rules. Upside: Separate JSDoc.

Copy link
Member Author

Choose a reason for hiding this comment

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

I split them up and ignored this eslint rule there!


/**
* Fire a hook event to process events before they are passed to (global) event processors.
* Expects to be given an Event & EventHint as the second/third argument.
*/
emit?(hook: 'preprocessEvent', event: Event, hint?: EventHint): void;

/*
* Fire a hook event after sending an event. Expects to be given an Event as the
* second argument.
Expand All @@ -245,4 +275,6 @@ export interface Client<O extends ClientOptions = ClientOptions> {
* The option argument may be mutated to drop the span.
*/
emit?(hook: 'otelSpanEnd', otelSpan: unknown, mutableOptions: { drop: boolean }): void;

/* eslint-enable @typescript-eslint/unified-signatures */
}
7 changes: 7 additions & 0 deletions packages/types/src/integration.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import type { Client } from './client';
import type { Event, EventHint } from './event';
import type { EventProcessor } from './eventprocessor';
import type { Hub } from './hub';

Expand All @@ -23,4 +25,9 @@ export interface Integration {
* This takes no options on purpose, options should be passed in the constructor
*/
setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void, getCurrentHub: () => Hub): void;

/**
* An optional hook that allows to preprocess an event _before_ it is passed to all other event processors.
*/
preprocessEvent?(event: Event, hint: EventHint | undefined, client: Client): void;
}