Skip to content
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ const defaultOptions: ParsedOptions = {
},
},
collectors: [],
errorFilters: [],
};

it('sends buffered events when client is registered', () => {
Expand Down Expand Up @@ -534,3 +535,125 @@ it('sends session init event when client is registered', () => {
}),
);
});

it('applies error filters to captured errors', () => {
const options: ParsedOptions = {
...defaultOptions,
errorFilters: [
(error) => ({
...error,
message: error.message.replace('secret', 'redacted'),
}),
],
};
const telemetry = new BrowserTelemetryImpl(options);

telemetry.captureError(new Error('Error with secret info'));
telemetry.register(mockClient);

expect(mockClient.track).toHaveBeenCalledWith(
'$ld:telemetry:error',
expect.objectContaining({
message: 'Error with redacted info',
}),
);
});

it('filters out errors when filter returns undefined', () => {
const options: ParsedOptions = {
...defaultOptions,
errorFilters: [() => undefined],
};
const telemetry = new BrowserTelemetryImpl(options);

telemetry.captureError(new Error('Test error'));
telemetry.register(mockClient);

// Verify only session init event was tracked
expect(mockClient.track).toHaveBeenCalledTimes(1);
expect(mockClient.track).toHaveBeenCalledWith(
'$ld:telemetry:session:init',
expect.objectContaining({
sessionId: expect.any(String),
}),
);
});

it('applies multiple error filters in sequence', () => {
const options: ParsedOptions = {
...defaultOptions,
errorFilters: [
(error) => ({
...error,
message: error.message.replace('secret', 'redacted'),
}),
(error) => ({
...error,
message: error.message.replace('redacted', 'sneaky'),
}),
],
};
const telemetry = new BrowserTelemetryImpl(options);

telemetry.captureError(new Error('Error with secret info'));
telemetry.register(mockClient);

expect(mockClient.track).toHaveBeenCalledWith(
'$ld:telemetry:error',
expect.objectContaining({
message: 'Error with sneaky info',
}),
);
});

it('handles error filter throwing an exception', () => {
const mockLogger = {
warn: jest.fn(),
};
const options: ParsedOptions = {
...defaultOptions,
errorFilters: [
() => {
throw new Error('Filter error');
},
],
logger: mockLogger,
};
const telemetry = new BrowserTelemetryImpl(options);

telemetry.captureError(new Error('Test error'));
telemetry.register(mockClient);

expect(mockLogger.warn).toHaveBeenCalledWith(
'LaunchDarkly - Browser Telemetry: Error applying error filters: Error: Filter error',
);
// Verify only session init event was tracked
expect(mockClient.track).toHaveBeenCalledTimes(1);
expect(mockClient.track).toHaveBeenCalledWith(
'$ld:telemetry:session:init',
expect.objectContaining({
sessionId: expect.any(String),
}),
);
});

it('only logs error filter error once', () => {
const mockLogger = {
warn: jest.fn(),
};
const options: ParsedOptions = {
...defaultOptions,
errorFilters: [
() => {
throw new Error('Filter error');
},
],
logger: mockLogger,
};
const telemetry = new BrowserTelemetryImpl(options);

telemetry.captureError(new Error('Error 1'));
telemetry.captureError(new Error('Error 2'));

expect(mockLogger.warn).toHaveBeenCalledTimes(1);
});
38 changes: 35 additions & 3 deletions packages/telemetry/browser-telemetry/__tests__/options.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Breadcrumb } from '../src/api/Breadcrumb';
import { ErrorData } from '../src/api/ErrorData';
import ErrorCollector from '../src/collectors/error';
import parse, { defaultOptions } from '../src/options';

Expand All @@ -16,17 +17,19 @@ it('handles an empty configuration', () => {
});

it('can set all options at once', () => {
const filter = (breadcrumb: Breadcrumb) => breadcrumb;
const breadcrumbFilter = (breadcrumb: Breadcrumb) => breadcrumb;
const errorFilter = (error: ErrorData) => error;
const outOptions = parse({
maxPendingEvents: 1,
breadcrumbs: {
maxBreadcrumbs: 1,
click: false,
evaluations: false,
flagChange: false,
filters: [filter],
filters: [breadcrumbFilter],
},
collectors: [new ErrorCollector(), new ErrorCollector()],
errorFilters: [errorFilter],
});
expect(outOptions).toEqual({
maxPendingEvents: 1,
Expand All @@ -41,7 +44,7 @@ it('can set all options at once', () => {
instrumentFetch: true,
instrumentXhr: true,
},
filters: expect.arrayContaining([filter]),
filters: expect.arrayContaining([breadcrumbFilter]),
},
stack: {
source: {
Expand All @@ -51,6 +54,7 @@ it('can set all options at once', () => {
},
},
collectors: [new ErrorCollector(), new ErrorCollector()],
errorFilters: expect.arrayContaining([errorFilter]),
Copy link
Member Author

Choose a reason for hiding this comment

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

The breadcrumbs filter is scoped to breadcrumbs, so it can just be called filters, but this is top level configuration so I think it needs error prepended.

});
expect(mockLogger.warn).not.toHaveBeenCalled();
});
Expand Down Expand Up @@ -441,3 +445,31 @@ it('warns when filters is not an array', () => {
'LaunchDarkly - Browser Telemetry: Config option "breadcrumbs.filters" should be of type BreadcrumbFilter[], got string, using default value',
);
});

it('warns when errorFilters is not an array', () => {
const outOptions = parse(
{
// @ts-ignore
errorFilters: 'not an array',
},
mockLogger,
);

expect(outOptions.errorFilters).toEqual([]);
expect(mockLogger.warn).toHaveBeenCalledWith(
'LaunchDarkly - Browser Telemetry: Config option "errorFilters" should be of type ErrorDataFilter[], got string, using default value',
);
});

it('accepts valid error filters array', () => {
const errorFilters = [(error: any) => error];
const outOptions = parse(
{
errorFilters,
},
mockLogger,
);

expect(outOptions.errorFilters).toEqual(errorFilters);
expect(mockLogger.warn).not.toHaveBeenCalled();
});
60 changes: 38 additions & 22 deletions packages/telemetry/browser-telemetry/src/BrowserTelemetryImpl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
import type { LDContext, LDEvaluationDetail } from '@launchdarkly/js-client-sdk';

import { BreadcrumbFilter, LDClientLogging, LDClientTracking, MinLogger } from './api';
import { LDClientLogging, LDClientTracking, MinLogger } from './api';
import { Breadcrumb, FeatureManagementBreadcrumb } from './api/Breadcrumb';
import { BrowserTelemetry } from './api/BrowserTelemetry';
import { BrowserTelemetryInspector } from './api/client/BrowserTelemetryInspector';
Expand Down Expand Up @@ -54,11 +54,8 @@ function safeValue(u: unknown): string | boolean | number | undefined {
}
}

function applyBreadcrumbFilter(
breadcrumb: Breadcrumb | undefined,
filter: BreadcrumbFilter,
): Breadcrumb | undefined {
return breadcrumb === undefined ? undefined : filter(breadcrumb);
function applyFilter<T>(item: T | undefined, filter: (item: T) => T | undefined): T | undefined {
return item === undefined ? undefined : filter(item);
}

function configureTraceKit(options: ParsedStackOptions) {
Expand All @@ -69,7 +66,7 @@ function configureTraceKit(options: ParsedStackOptions) {
// from the before context.
// The typing for this is a bool, but it accepts a number.
const beforeAfterMax = Math.max(options.source.afterLines, options.source.beforeLines);
// The assignment here has bene split to prevent esbuild from complaining about an assigment to
// The assignment here has bene split to prevent esbuild from complaining about an assignment to
// an import. TraceKit exports a single object and the interface requires modifying an exported
// var.
const anyObj = TraceKit as any;
Expand Down Expand Up @@ -105,6 +102,8 @@ export default class BrowserTelemetryImpl implements BrowserTelemetry {
private _eventsDropped: boolean = false;
// Used to ensure we only log the breadcrumb filter error once.
private _breadcrumbFilterError: boolean = false;
// Used to ensure we only log the error filter error once.
private _errorFilterError: boolean = false;

constructor(private _options: ParsedOptions) {
configureTraceKit(_options.stack);
Expand Down Expand Up @@ -198,8 +197,18 @@ export default class BrowserTelemetryImpl implements BrowserTelemetry {
* @param event The event data.
*/
private _capture(type: string, event: EventData) {
const filteredEvent = this._applyFilters(event, this._options.errorFilters, (e: unknown) => {
if (!this._errorFilterError) {
this._errorFilterError = true;
this._logger.warn(prefixLog(`Error applying error filters: ${e}`));
Copy link
Contributor

Choose a reason for hiding this comment

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

Does e need to be json stringified in order to print the value instead of [Object object] or the type?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see this code was just moved from a catch to here. I'm still curious if this logs helpful information, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Generally no, as long as it is actually an error or a string. The default formatting for the error type will use the name and message (https://tc39.es/ecma262/multipage/fundamental-objects.html#sec-error.prototype.tostring).

Sometimes it is good to have custom error formatting when you aren't certain it will be an error.

There is some additional complexity generally with errors as they tend to either be self-referential, or have serialization which results in strange field output.

Just logging an error will typically use the message.

> console.log(`Error ${new Error("potato")}`)
Error Error: potato

JSON.stringify an error.

JSON.stringify(new Error("potato"))
'{}'

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah! Gotcha, thanks for explaining.

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 will make a task to see if I can safely add some meta-data about the specific filter. If nothing else potentially the index.

Copy link
Contributor

Choose a reason for hiding this comment

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

is e always unknown here or can it be typed to an Error if it's always called from a try-catch?

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 could add a type guard using instanceof, but the typing of the catch is any/unknown depending on ts version. You technically can do really annoying things like throw 10.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh dang, TIL. Makes sense why you've implemented it as an unknown type.

}
});
if (filteredEvent === undefined) {
return;
}

if (this._client === undefined) {
this._pendingEvents.push({ type, data: event });
this._pendingEvents.push({ type, data: filteredEvent });
if (this._pendingEvents.length > this._maxPendingEvents) {
if (!this._eventsDropped) {
this._eventsDropped = true;
Expand All @@ -212,7 +221,7 @@ export default class BrowserTelemetryImpl implements BrowserTelemetry {
this._pendingEvents.shift();
}
}
this._client?.track(type, event);
this._client?.track(type, filteredEvent);
}

captureError(exception: Error): void {
Expand Down Expand Up @@ -241,27 +250,34 @@ export default class BrowserTelemetryImpl implements BrowserTelemetry {
this.captureError(errorEvent.error);
}

private _applyBreadcrumbFilters(
breadcrumb: Breadcrumb,
filters: BreadcrumbFilter[],
): Breadcrumb | undefined {
private _applyFilters<T>(
item: T,
filters: ((item: T) => T | undefined)[],
handleError: (e: unknown) => void,
): T | undefined {
try {
return filters.reduce(
(breadcrumbToFilter: Breadcrumb | undefined, filter: BreadcrumbFilter) =>
applyBreadcrumbFilter(breadcrumbToFilter, filter),
breadcrumb,
(itemToFilter: T | undefined, filter: (item: T) => T | undefined) =>
applyFilter(itemToFilter, filter),
item,
);
} catch (e) {
if (!this._breadcrumbFilterError) {
this._breadcrumbFilterError = true;
this._logger.warn(prefixLog(`Error applying breadcrumb filters: ${e}`));
}
handleError(e);
return undefined;
}
}

addBreadcrumb(breadcrumb: Breadcrumb): void {
const filtered = this._applyBreadcrumbFilters(breadcrumb, this._options.breadcrumbs.filters);
const filtered = this._applyFilters(
breadcrumb,
this._options.breadcrumbs.filters,
(e: unknown) => {
if (!this._breadcrumbFilterError) {
this._breadcrumbFilterError = true;
this._logger.warn(prefixLog(`Error applying breadcrumb filters: ${e}`));
}
},
);
if (filtered !== undefined) {
this._breadcrumbs.push(filtered);
if (this._breadcrumbs.length > this._maxBreadcrumbs) {
Expand All @@ -275,7 +291,7 @@ export default class BrowserTelemetryImpl implements BrowserTelemetry {
}

/**
* Used to automatically collect flag usage for breacrumbs.
* Used to automatically collect flag usage for breadcrumbs.
*
* When session replay is in use the data is also forwarded to the session
* replay collector.
Expand Down
23 changes: 21 additions & 2 deletions packages/telemetry/browser-telemetry/src/api/Options.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Breadcrumb } from './Breadcrumb';
import { Collector } from './Collector';
import { ErrorData } from './ErrorData';
import { MinLogger } from './MinLogger';

/**
Expand Down Expand Up @@ -27,13 +28,21 @@ export interface UrlFilter {
/**
* Interface for breadcrumb filters.
*
* Given a breadcrumb the filter may return a modified breadcrumb or undefined to
* exclude the breadcrumb.
* Given a breadcrumb the filter may return a modified breadcrumb or undefined to exclude the breadcrumb.
*/
export interface BreadcrumbFilter {
(breadcrumb: Breadcrumb): Breadcrumb | undefined;
}

/**
* Interface for filtering error data before it is sent to LaunchDarkly.
*
* Given {@link ErrorData} the filter may return modified data or undefined to exclude the breadcrumb.
*/
export interface ErrorDataFilter {
(event: ErrorData): ErrorData | undefined;
}

export interface HttpBreadcrumbOptions {
/**
* If fetch should be instrumented and breadcrumbs included for fetch requests.
Expand Down Expand Up @@ -197,4 +206,14 @@ export interface Options {
* logger. The 3.x SDKs do not expose their logger.
*/
logger?: MinLogger;

/**
* Custom error data filters.
*
* Can be used to redact or modify error data.
*
* For filtering breadcrumbs or URLs in error data, see {@link breadcrumbs.filters} and
* {@link breadcrumbs.http.customUrlFilter}.
*/
errorFilters?: ErrorDataFilter[];
}
Loading
Loading