Skip to content

ref: Enforce stackParser through options. #4953

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 21, 2022
Merged

ref: Enforce stackParser through options. #4953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d42fb00
ref: Enforce stackParser through options.
AbhiPrasad Apr 19, 2022
503ea25
make client type return more clear
AbhiPrasad Apr 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 3 additions & 9 deletions packages/browser/src/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { BaseClient, NewTransport, Scope, SDK_VERSION } from '@sentry/core';
import { ClientOptions, Event, EventHint, Options, Severity, SeverityLevel, Transport } from '@sentry/types';
import { getGlobalObject, logger, stackParserFromOptions } from '@sentry/utils';
import { getGlobalObject, logger } from '@sentry/utils';

import { eventFromException, eventFromMessage } from './eventbuilder';
import { IS_DEBUG_BUILD } from './flags';
Expand Down Expand Up @@ -89,7 +89,7 @@ export class BrowserClient extends BaseClient<BrowserClientOptions> {
* @inheritDoc
*/
public eventFromException(exception: unknown, hint?: EventHint): PromiseLike<Event> {
return eventFromException(stackParserFromOptions(this._options), exception, hint, this._options.attachStacktrace);
return eventFromException(this._options.stackParser, exception, hint, this._options.attachStacktrace);
}

/**
Expand All @@ -101,13 +101,7 @@ export class BrowserClient extends BaseClient<BrowserClientOptions> {
level: Severity | SeverityLevel = 'info',
hint?: EventHint,
): PromiseLike<Event> {
return eventFromMessage(
stackParserFromOptions(this._options),
message,
level,
hint,
this._options.attachStacktrace,
);
return eventFromMessage(this._options.stackParser, message, level, hint, this._options.attachStacktrace);
}

/**
Expand Down
10 changes: 5 additions & 5 deletions packages/browser/src/integrations/globalhandlers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import {
isPrimitive,
isString,
logger,
stackParserFromOptions,
} from '@sentry/utils';

import { BrowserClient } from '../client';
Expand Down Expand Up @@ -255,8 +254,9 @@ function addMechanismAndCapture(hub: Hub, error: EventHint['originalException'],
function getHubAndOptions(): [Hub, StackParser, boolean | undefined] {
const hub = getCurrentHub();
const client = hub.getClient<BrowserClient>();
const options = client?.getOptions();
const parser = stackParserFromOptions(options);
const attachStacktrace = options?.attachStacktrace;
return [hub, parser, attachStacktrace];
const options = (client && client.getOptions()) || {
stackParser: () => [],
attachStacktrace: false,
};
return [hub, options.stackParser, options.attachStacktrace];
}
11 changes: 6 additions & 5 deletions packages/browser/src/integrations/linkederrors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { addGlobalEventProcessor, getCurrentHub } from '@sentry/core';
import { Event, EventHint, Exception, ExtendedError, Integration, StackParser } from '@sentry/types';
import { isInstanceOf, stackParserFromOptions } from '@sentry/utils';
import { isInstanceOf } from '@sentry/utils';

import { BrowserClient } from '../client';
import { exceptionFromError } from '../eventbuilder';
Expand Down Expand Up @@ -47,12 +47,13 @@ export class LinkedErrors implements Integration {
* @inheritDoc
*/
public setupOnce(): void {
const options = getCurrentHub().getClient<BrowserClient>()?.getOptions();
const parser = stackParserFromOptions(options);

const client = getCurrentHub().getClient<BrowserClient>();
if (!client) {
return;
}
addGlobalEventProcessor((event: Event, hint?: EventHint) => {
const self = getCurrentHub().getIntegration(LinkedErrors);
return self ? _handler(parser, self._key, self._limit, event, hint) : event;
return self ? _handler(client.getOptions().stackParser, self._key, self._limit, event, hint) : event;
});
}
}
Expand Down
5 changes: 1 addition & 4 deletions packages/browser/src/sdk.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,14 +102,11 @@ export function init(options: BrowserOptions = {}): void {
if (options.sendClientReports === undefined) {
options.sendClientReports = true;
}
if (options.stackParser === undefined) {
options.stackParser = defaultStackParsers;
}
const { transport, newTransport } = setupBrowserTransport(options);

const clientOptions: BrowserClientOptions = {
...options,
stackParser: stackParserFromOptions(options),
stackParser: stackParserFromOptions(options.stackParser || defaultStackParsers),
integrations: getIntegrationsToSetup(options),
// TODO(v7): get rid of transport being passed down below
transport: options.transport || (supportsFetch() ? FetchTransport : XHRTransport),
Expand Down
6 changes: 3 additions & 3 deletions packages/node/src/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { BaseClient, NewTransport, Scope, SDK_VERSION } from '@sentry/core';
import { SessionFlusher } from '@sentry/hub';
import { Event, EventHint, Severity, SeverityLevel, Transport } from '@sentry/types';
import { logger, resolvedSyncPromise, stackParserFromOptions } from '@sentry/utils';
import { logger, resolvedSyncPromise } from '@sentry/utils';

import { eventFromMessage, eventFromUnknownInput } from './eventbuilder';
import { IS_DEBUG_BUILD } from './flags';
Expand Down Expand Up @@ -111,7 +111,7 @@ export class NodeClient extends BaseClient<NodeClientOptions> {
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types
public eventFromException(exception: any, hint?: EventHint): PromiseLike<Event> {
return resolvedSyncPromise(eventFromUnknownInput(stackParserFromOptions(this._options), exception, hint));
return resolvedSyncPromise(eventFromUnknownInput(this._options.stackParser, exception, hint));
}

/**
Expand All @@ -124,7 +124,7 @@ export class NodeClient extends BaseClient<NodeClientOptions> {
hint?: EventHint,
): PromiseLike<Event> {
return resolvedSyncPromise(
eventFromMessage(stackParserFromOptions(this._options), message, level, hint, this._options.attachStacktrace),
eventFromMessage(this._options.stackParser, message, level, hint, this._options.attachStacktrace),
);
}

Expand Down
10 changes: 4 additions & 6 deletions packages/node/src/integrations/linkederrors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { addGlobalEventProcessor, getCurrentHub } from '@sentry/core';
import { Event, EventHint, Exception, ExtendedError, Integration, StackParser } from '@sentry/types';
import { isInstanceOf, resolvedSyncPromise, stackParserFromOptions, SyncPromise } from '@sentry/utils';
import { isInstanceOf, resolvedSyncPromise, SyncPromise } from '@sentry/utils';

import { NodeClient } from '../client';
import { exceptionFromError } from '../eventbuilder';
Expand Down Expand Up @@ -46,12 +46,10 @@ export class LinkedErrors implements Integration {
addGlobalEventProcessor(async (event: Event, hint?: EventHint) => {
const hub = getCurrentHub();
const self = hub.getIntegration(LinkedErrors);
const stackParser = stackParserFromOptions(hub.getClient<NodeClient>()?.getOptions());

if (self) {
await self._handler(stackParser, event, hint);
const client = hub.getClient<NodeClient>();
if (client && self) {
await self._handler(client.getOptions().stackParser, event, hint);
}

return event;
});
}
Expand Down
6 changes: 1 addition & 5 deletions packages/node/src/sdk.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,10 +122,6 @@ export function init(options: NodeOptions = {}): void {
options.autoSessionTracking = true;
}

if (options.stackParser === undefined) {
options.stackParser = [nodeStackParser];
}

// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access, @typescript-eslint/no-explicit-any
if ((domain as any).active) {
setHubOnCarrier(carrier, getCurrentHub());
Expand All @@ -136,7 +132,7 @@ export function init(options: NodeOptions = {}): void {
// TODO(v7): Refactor this to reduce the logic above
const clientOptions: NodeClientOptions = {
...options,
stackParser: stackParserFromOptions(options),
stackParser: stackParserFromOptions(options.stackParser || [nodeStackParser]),
integrations: getIntegrationsToSetup(options),
// TODO(v7): Fix me when we switch to new transports entirely.
transport: options.transport || (transport instanceof HTTPTransport ? HTTPTransport : HTTPSTransport),
Expand Down
22 changes: 6 additions & 16 deletions packages/utils/src/stacktrace.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,27 +30,17 @@ export function createStackParser(...parsers: StackLineParser[]): StackParser {
};
}

interface StackParserOptions {
stackParser?: StackParser | StackLineParser[];
}

/**
* Gets a stack parser implementation from options
* Gets a stack parser implementation from Options.stackParser
* @see Options
*
* If options contains an array of line parsers, it is converted into a parser
*/
export function stackParserFromOptions(options: StackParserOptions | undefined): StackParser {
if (options) {
if (Array.isArray(options.stackParser)) {
options.stackParser = createStackParser(...options.stackParser);
}

if (typeof options.stackParser === 'function') {
return options.stackParser;
}
export function stackParserFromOptions(stackParser: StackParser | StackLineParser[]): StackParser {
Comment on lines +34 to +39
Copy link
Member

Choose a reason for hiding this comment

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

After this change, seems like it's not being gotten from options anymore. Can we change the name to reflect that?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a good point, and has got me thinking if we even should expose passing in a StackLineParser[], and enforce that it is always an (optional) StackParser.

Copy link
Collaborator

@timfish timfish Apr 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The only plus to allow passing of StackLineParser[] is that users can configure stack parsing without needing to import and use createStackParser from @sentry/utils.

Some common lints warn if you use modules that are not direct dependencies so users would also need to add @sentry/utils to their depndencies or we would need to be re-export it from every sdk.

Allowing user passing of StackParser allows completely overridding the entire stack parsing logic. I'm not sure we have a use case for this yet.

if (Array.isArray(stackParser)) {
return createStackParser(...stackParser);
}

return _ => [];
return stackParser;
}

/**
Expand Down