Skip to content

Commit

Permalink
ref(node): Refactor node source fetching into integration (#3729)
Browse files Browse the repository at this point in the history
- Moves `eventFromException` and `eventFromMessage` into `eventbuilder` to match browser SDK
- Moves source context loading/addition into a `ContextLines` integration
- `frameContextLines`  option gets copied to `ContextLines` constructor but also pulls `frameContextLines` from `NodeOptions` to avoid a breaking change. 
- `frameContextLines` in `NodeOptions` marked as `@deprecated` so users learn about future migration
  • Loading branch information
timfish committed Feb 23, 2022
1 parent 8bcd867 commit c60556f
Show file tree
Hide file tree
Showing 10 changed files with 266 additions and 259 deletions.
8 changes: 4 additions & 4 deletions packages/nextjs/test/index.server.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { BaseClient } from '@sentry/core';
import { RewriteFrames } from '@sentry/integrations';
import * as SentryNode from '@sentry/node';
import { getCurrentHub, NodeClient } from '@sentry/node';
Expand All @@ -17,7 +16,6 @@ const global = getGlobalObject();
(global as typeof global & { __rewriteFramesDistDir__: string }).__rewriteFramesDistDir__ = '.next';

const nodeInit = jest.spyOn(SentryNode, 'init');
const captureEvent = jest.spyOn(BaseClient.prototype, 'captureEvent');
const logError = jest.spyOn(logger, 'error');

describe('Server init()', () => {
Expand Down Expand Up @@ -91,7 +89,7 @@ describe('Server init()', () => {
expect(currentScope._tags.vercel).toBeUndefined();
});

it('adds 404 transaction filter', () => {
it('adds 404 transaction filter', async () => {
init({
dsn: 'https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012',
tracesSampleRate: 1.0,
Expand All @@ -102,8 +100,10 @@ describe('Server init()', () => {
const transaction = hub.startTransaction({ name: '/404' });
transaction.finish();

// We need to flush because the event processor pipeline is async whereas transaction.finish() is sync.
await SentryNode.flush();

expect(sendEvent).not.toHaveBeenCalled();
expect(captureEvent.mock.results[0].value).toBeUndefined();
expect(logError).toHaveBeenCalledWith(new SentryError('An event processor returned null, will not send event.'));
});

Expand Down
2 changes: 1 addition & 1 deletion packages/node/src/backend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export class NodeBackend extends BaseBackend<NodeOptions> {
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types
public eventFromException(exception: any, hint?: EventHint): PromiseLike<Event> {
return eventFromException(this._options, exception, hint);
return eventFromException(exception, hint);
}

/**
Expand Down
19 changes: 7 additions & 12 deletions packages/node/src/eventbuilder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { extractStackFromError, parseError, parseStack, prepareFramesForEvent }
* Builds and Event from a Exception
* @hidden
*/
export function eventFromException(options: Options, exception: unknown, hint?: EventHint): PromiseLike<Event> {
export function eventFromException(exception: unknown, hint?: EventHint): PromiseLike<Event> {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
let ex: any = exception;
const providedMechanism: Mechanism | undefined =
Expand Down Expand Up @@ -48,7 +48,7 @@ export function eventFromException(options: Options, exception: unknown, hint?:
}

return new SyncPromise<Event>((resolve, reject) =>
parseError(ex as Error, options)
parseError(ex as Error)
.then(event => {
addExceptionTypeValue(event, undefined, undefined);
addExceptionMechanism(event, mechanism);
Expand Down Expand Up @@ -81,16 +81,11 @@ export function eventFromMessage(
return new SyncPromise<Event>(resolve => {
if (options.attachStacktrace && hint && hint.syntheticException) {
const stack = hint.syntheticException ? extractStackFromError(hint.syntheticException) : [];
void parseStack(stack, options)
.then(frames => {
event.stacktrace = {
frames: prepareFramesForEvent(frames),
};
resolve(event);
})
.then(null, () => {
resolve(event);
});
const frames = parseStack(stack);
event.stacktrace = {
frames: prepareFramesForEvent(frames),
};
resolve(event);
} else {
resolve(event);
}
Expand Down
140 changes: 140 additions & 0 deletions packages/node/src/integrations/contextlines.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
import { getCurrentHub } from '@sentry/core';
import { Event, EventProcessor, Integration } from '@sentry/types';
import { addContextToFrame } from '@sentry/utils';
import { readFile } from 'fs';
import { LRUMap } from 'lru_map';

import { NodeClient } from '../client';

const FILE_CONTENT_CACHE = new LRUMap<string, string | null>(100);
const DEFAULT_LINES_OF_CONTEXT = 7;

// TODO: Replace with promisify when minimum supported node >= v8
function readTextFileAsync(path: string): Promise<string> {
return new Promise((resolve, reject) => {
readFile(path, 'utf8', (err, data) => {
if (err) reject(err);
else resolve(data);
});
});
}

/**
* Resets the file cache. Exists for testing purposes.
* @hidden
*/
export function resetFileContentCache(): void {
FILE_CONTENT_CACHE.clear();
}

interface ContextLinesOptions {
/**
* Sets the number of context lines for each frame when loading a file.
* Defaults to 7.
*
* Set to 0 to disable loading and inclusion of source files.
**/
frameContextLines?: number;
}

/** Add node modules / packages to the event */
export class ContextLines implements Integration {
/**
* @inheritDoc
*/
public static id: string = 'ContextLines';

/**
* @inheritDoc
*/
public name: string = ContextLines.id;

public constructor(private readonly _options: ContextLinesOptions = {}) {}

/**
* @inheritDoc
*/
public setupOnce(addGlobalEventProcessor: (callback: EventProcessor) => void): void {
// This is only here to copy frameContextLines from init options if it hasn't
// been set via this integrations constructor.
//
// TODO: Remove on next major!
if (this._options.frameContextLines === undefined) {
const initOptions = getCurrentHub().getClient<NodeClient>()?.getOptions();
// eslint-disable-next-line deprecation/deprecation
this._options.frameContextLines = initOptions?.frameContextLines;
}

const contextLines =
this._options.frameContextLines !== undefined ? this._options.frameContextLines : DEFAULT_LINES_OF_CONTEXT;

addGlobalEventProcessor(event => this.addSourceContext(event, contextLines));
}

/** Processes an event and adds context lines */
public async addSourceContext(event: Event, contextLines: number): Promise<Event> {
const frames = event.exception?.values?.[0].stacktrace?.frames;

if (frames && contextLines > 0) {
const filenames: Set<string> = new Set();

for (const frame of frames) {
if (frame.filename) {
filenames.add(frame.filename);
}
}

const sourceFiles = await readSourceFiles(filenames);

for (const frame of frames) {
if (frame.filename && sourceFiles[frame.filename]) {
try {
const lines = (sourceFiles[frame.filename] as string).split('\n');
addContextToFrame(lines, frame, contextLines);
} catch (e) {
// anomaly, being defensive in case
// unlikely to ever happen in practice but can definitely happen in theory
}
}
}
}

return event;
}
}

/**
* This function reads file contents and caches them in a global LRU cache.
*
* @param filenames Array of filepaths to read content from.
*/
async function readSourceFiles(filenames: Set<string>): Promise<Record<string, string | null>> {
const sourceFiles: Record<string, string | null> = {};

for (const filename of filenames) {
const cachedFile = FILE_CONTENT_CACHE.get(filename);
// We have a cache hit
if (cachedFile !== undefined) {
// If stored value is null, it means that we already tried, but couldn't read the content of the file. Skip.
if (cachedFile === null) {
continue;
}

// Otherwise content is there, so reuse cached value.
sourceFiles[filename] = cachedFile;
continue;
}

let content: string | null = null;
try {
content = await readTextFileAsync(filename);
} catch (_) {
//
}

FILE_CONTENT_CACHE.set(filename, content);
sourceFiles[filename] = content;
}

return sourceFiles;
}
1 change: 1 addition & 0 deletions packages/node/src/integrations/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@ export { OnUncaughtException } from './onuncaughtexception';
export { OnUnhandledRejection } from './onunhandledrejection';
export { LinkedErrors } from './linkederrors';
export { Modules } from './modules';
export { ContextLines } from './contextlines';

0 comments on commit c60556f

Please sign in to comment.