Skip to content

ref: Record dropped events in BaseClient #5017

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 4 commits into from
Apr 29, 2022
Merged

ref: Record dropped events in BaseClient #5017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
152a8ec
Make `sendEnvelope` private
lforst Apr 28, 2022
f0d21c2
ref: Record dropped events in BaseClient
lforst Apr 29, 2022
c2e57d2
Only accumulate clientRecords when `sendClientReports` is true
lforst Apr 29, 2022
9b69ea2
Fix test
lforst Apr 29, 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
81 changes: 55 additions & 26 deletions packages/core/src/baseclient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,15 @@ import { Scope, Session } from '@sentry/hub';
import {
Client,
ClientOptions,
DataCategory,
DsnComponents,
Envelope,
Event,
EventDropReason,
EventHint,
Integration,
IntegrationClass,
Outcome,
SessionAggregates,
Severity,
SeverityLevel,
Expand Down Expand Up @@ -87,6 +90,9 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
/** Number of calls being processed */
protected _numProcessing: number = 0;

/** Holds flushable */
private _outcomes: { [key: string]: number } = {};

/**
* Initializes this client instance.
*
Expand All @@ -98,7 +104,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
this._dsn = makeDsn(options.dsn);
const url = getEnvelopeEndpointWithUrlEncodedAuth(this._dsn, options.tunnel);
this._transport = options.transport({
recordDroppedEvent: () => undefined, // TODO(v7): Provide a proper function instead of noop
recordDroppedEvent: this.recordDroppedEvent.bind(this),
...options.transportOptions,
url,
});
Expand Down Expand Up @@ -270,7 +276,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
public sendEvent(event: Event): void {
if (this._dsn) {
const env = createEventEnvelope(event, this._dsn, this._options._metadata, this._options.tunnel);
this.sendEnvelope(env);
this._sendEnvelope(env);
}
}

Expand All @@ -280,20 +286,26 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
public sendSession(session: Session | SessionAggregates): void {
if (this._dsn) {
const env = createSessionEnvelope(session, this._dsn, this._options._metadata, this._options.tunnel);
this.sendEnvelope(env);
this._sendEnvelope(env);
}
}

/**
* @inheritdoc
* @inheritDoc
*/
public sendEnvelope(env: Envelope): void {
if (this._transport && this._dsn) {
this._transport.send(env).then(null, reason => {
IS_DEBUG_BUILD && logger.error('Error while sending event:', reason);
});
} else {
IS_DEBUG_BUILD && logger.error('Transport disabled');
public recordDroppedEvent(reason: EventDropReason, category: DataCategory): void {
Copy link
Member

Choose a reason for hiding this comment

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

This should only work if the sendClientReports option is true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can get away with recording the events, but only actually sending them when sendClientReports is true. At least that's how I envisioned it with #5018.

I would also find it a tad cleaner from a single-responsibility perspective. Wdyt? I'll let you decide 😄

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 we can get away with recording the events, but only actually sending them when sendClientReports is true

I was thinking problems with integer overflow or outcomes for long running applications (like a node server), but maybe not that big of a deal with what you showed before.

if (this._options.sendClientReports) {
// We want to track each category (error, transaction, session) separately
// but still keep the distinction between different type of outcomes.
// We could use nested maps, but it's much easier to read and type this way.
// A correct type for map-based implementation if we want to go that route
// would be `Partial<Record<SentryRequestType, Partial<Record<Outcome, number>>>>`
// With typescript 4.1 we could even use template literal types
const key = `${reason}:${category}`;
IS_DEBUG_BUILD && logger.log(`Adding outcome: "${key}"`);

// The following works because undefined + 1 === NaN and NaN is falsy
this._outcomes[key] = this._outcomes[key] + 1 || 1;
}
}

Expand Down Expand Up @@ -565,20 +577,8 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
* @returns A SyncPromise that resolves with the event or rejects in case event was/will not be send.
*/
protected _processEvent(event: Event, hint?: EventHint, scope?: Scope): PromiseLike<Event> {
// eslint-disable-next-line @typescript-eslint/unbound-method
const { beforeSend, sampleRate } = this.getOptions();

// type RecordLostEvent = NonNullable<Transport['recordLostEvent']>;
type RecordLostEventParams = unknown[]; // Parameters<RecordLostEvent>;

// no-op as new transports don't have client outcomes
// TODO(v7): Re-add this functionality
function recordLostEvent(_outcome: RecordLostEventParams[0], _category: RecordLostEventParams[1]): void {
// if (transport.recordLostEvent) {
// transport.recordLostEvent(outcome, category);
// }
}

if (!this._isEnabled()) {
return rejectedSyncPromise(new SentryError('SDK not enabled, will not capture event.'));
}
Expand All @@ -588,7 +588,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
// 0.0 === 0% events are sent
// Sampling for transaction happens somewhere else
if (!isTransaction && typeof sampleRate === 'number' && Math.random() > sampleRate) {
recordLostEvent('sample_rate', 'event');
this.recordDroppedEvent('sample_rate', 'error');
return rejectedSyncPromise(
new SentryError(
`Discarding event because it's not included in the random sample (sampling rate = ${sampleRate})`,
Expand All @@ -599,7 +599,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
return this._prepareEvent(event, scope, hint)
.then(prepared => {
if (prepared === null) {
recordLostEvent('event_processor', event.type || 'event');
this.recordDroppedEvent('event_processor', event.type || 'error');
throw new SentryError('An event processor returned null, will not send event.');
}

Expand All @@ -613,7 +613,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
})
.then(processedEvent => {
if (processedEvent === null) {
recordLostEvent('before_send', event.type || 'event');
this.recordDroppedEvent('before_send', event.type || 'error');
throw new SentryError('`beforeSend` returned `null`, will not send event.');
}

Expand Down Expand Up @@ -659,6 +659,35 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
);
}

/**
* @inheritdoc
*/
protected _sendEnvelope(envelope: Envelope): void {
if (this._transport && this._dsn) {
this._transport.send(envelope).then(null, reason => {
IS_DEBUG_BUILD && logger.error('Error while sending event:', reason);
});
} else {
IS_DEBUG_BUILD && logger.error('Transport disabled');
}
}

/**
* Clears outcomes on this client and returns them.
*/
protected _clearOutcomes(): Outcome[] {
const outcomes = this._outcomes;
this._outcomes = {};
return Object.keys(outcomes).map(key => {
const [reason, category] = key.split(':') as [EventDropReason, DataCategory];
return {
reason,
category,
quantity: outcomes[key],
};
});
}

/**
* @inheritDoc
*/
Expand Down
157 changes: 101 additions & 56 deletions packages/core/test/lib/base.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1061,30 +1061,24 @@ describe('BaseClient', () => {
expect((TestClient.instance!.event! as any).data).toBe('someRandomThing');
});

// TODO(v7): Add back test with client reports
// test('beforeSend records dropped events', () => {
// expect.assertions(1);

// const client = new TestClient(
// getDefaultTestClientOptions({
// dsn: PUBLIC_DSN,
// beforeSend() {
// return null;
// },
// }),
// );
// const recordLostEventSpy = jest.fn();
// jest.spyOn(client, 'getTransport').mockImplementationOnce(
// () =>
// ({
// recordLostEvent: recordLostEventSpy,
// } as any as Transport),
// );

// client.captureEvent({ message: 'hello' }, {});

// expect(recordLostEventSpy).toHaveBeenCalledWith('before_send', 'event');
// });
test('beforeSend records dropped events', () => {
expect.assertions(1);

const client = new TestClient(
getDefaultTestClientOptions({
dsn: PUBLIC_DSN,
beforeSend() {
return null;
},
}),
);

const recordLostEventSpy = jest.spyOn(client, 'recordDroppedEvent');

client.captureEvent({ message: 'hello' }, {});

expect(recordLostEventSpy).toHaveBeenCalledWith('before_send', 'error');
});

test('eventProcessor can drop the even when it returns null', () => {
expect.assertions(3);
Expand All @@ -1102,28 +1096,21 @@ describe('BaseClient', () => {
expect(loggerErrorSpy).toBeCalledWith(new SentryError('An event processor returned null, will not send event.'));
});

// TODO(v7): Add back tests with client reports
// test('eventProcessor records dropped events', () => {
// expect.assertions(1);
test('eventProcessor records dropped events', () => {
expect.assertions(1);

// const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN });
// const client = new TestClient(options);
const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN });
const client = new TestClient(options);

// const recordLostEventSpy = jest.fn();
// jest.spyOn(client, 'getTransport').mockImplementationOnce(
// () =>
// ({
// recordLostEvent: recordLostEventSpy,
// } as any as Transport),
// );
const recordLostEventSpy = jest.spyOn(client, 'recordDroppedEvent');

// const scope = new Scope();
// scope.addEventProcessor(() => null);
const scope = new Scope();
scope.addEventProcessor(() => null);

// client.captureEvent({ message: 'hello' }, {}, scope);
client.captureEvent({ message: 'hello' }, {}, scope);

// expect(recordLostEventSpy).toHaveBeenCalledWith('event_processor', 'event');
// });
expect(recordLostEventSpy).toHaveBeenCalledWith('event_processor', 'error');
});

test('eventProcessor sends an event and logs when it crashes', () => {
expect.assertions(3);
Expand Down Expand Up @@ -1154,24 +1141,17 @@ describe('BaseClient', () => {
);
});

// TODO(v7): Add back test with client reports
// test('records events dropped due to sampleRate', () => {
// expect.assertions(1);
test('records events dropped due to sampleRate', () => {
expect.assertions(1);

// const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, sampleRate: 0 });
// const client = new TestClient(options);
const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, sampleRate: 0 });
const client = new TestClient(options);

// const recordLostEventSpy = jest.fn();
// jest.spyOn(client, 'getTransport').mockImplementationOnce(
// () =>
// ({
// recordLostEvent: recordLostEventSpy,
// } as any as Transport),
// );
const recordLostEventSpy = jest.spyOn(client, 'recordDroppedEvent');

// client.captureEvent({ message: 'hello' }, {});
// expect(recordLostEventSpy).toHaveBeenCalledWith('sample_rate', 'event');
// });
client.captureEvent({ message: 'hello' }, {});
expect(recordLostEventSpy).toHaveBeenCalledWith('sample_rate', 'error');
});
});

describe('integrations', () => {
Expand Down Expand Up @@ -1366,4 +1346,69 @@ describe('BaseClient', () => {
expect(TestClient.instance!.session).toBeUndefined();
});
});

describe('recordDroppedEvent()/_clearOutcomes()', () => {
test('record and return outcomes', () => {
const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN });
const client = new TestClient(options);

client.recordDroppedEvent('ratelimit_backoff', 'error');
client.recordDroppedEvent('ratelimit_backoff', 'error');
client.recordDroppedEvent('network_error', 'transaction');
client.recordDroppedEvent('network_error', 'transaction');
client.recordDroppedEvent('before_send', 'session');
client.recordDroppedEvent('event_processor', 'attachment');
client.recordDroppedEvent('network_error', 'transaction');

const clearedOutcomes = client._clearOutcomes();

expect(clearedOutcomes).toEqual(
expect.arrayContaining([
{
reason: 'ratelimit_backoff',
category: 'error',
quantity: 2,
},
{
reason: 'network_error',
category: 'transaction',
quantity: 3,
},
{
reason: 'before_send',
category: 'session',
quantity: 1,
},
{
reason: 'event_processor',
category: 'attachment',
quantity: 1,
},
]),
);
});

test('to clear outcomes', () => {
const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN });
const client = new TestClient(options);

client.recordDroppedEvent('ratelimit_backoff', 'error');
client.recordDroppedEvent('ratelimit_backoff', 'error');
client.recordDroppedEvent('event_processor', 'attachment');

const clearedOutcomes1 = client._clearOutcomes();
expect(clearedOutcomes1.length).toEqual(2);

const clearedOutcomes2 = client._clearOutcomes();
expect(clearedOutcomes2.length).toEqual(0);

client.recordDroppedEvent('network_error', 'attachment');

const clearedOutcomes3 = client._clearOutcomes();
expect(clearedOutcomes3.length).toEqual(1);

const clearedOutcomes4 = client._clearOutcomes();
expect(clearedOutcomes4.length).toEqual(0);
});
});
});
12 changes: 7 additions & 5 deletions packages/core/test/mocks/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,15 @@
import { Session } from '@sentry/hub';
import { ClientOptions, Event, Integration, Severity, SeverityLevel } from '@sentry/types';
import { ClientOptions, Event, Integration, Outcome, Severity, SeverityLevel } from '@sentry/types';
import { resolvedSyncPromise } from '@sentry/utils';

import { BaseClient } from '../../src/baseclient';
import { initAndBind } from '../../src/sdk';
import { createTransport } from '../../src/transports/base';

// TODO(v7): Add client reports tests to this file
// See old tests in packages/browser/test/unit/transports/base.test.ts
// from https://github.com/getsentry/sentry-javascript/pull/4967

export function getDefaultTestClientOptions(options: Partial<TestClientOptions> = {}): TestClientOptions {
return {
integrations: [],
sendClientReports: true,
transport: () =>
createTransport(
{ recordDroppedEvent: () => undefined }, // noop
Expand Down Expand Up @@ -79,6 +76,11 @@ export class TestClient extends BaseClient<TestClientOptions> {
public sendSession(session: Session): void {
this.session = session;
}

// Public proxy for protected method
public _clearOutcomes(): Outcome[] {
return super._clearOutcomes();
}
}

export function init(options: TestClientOptions): void {
Expand Down
Loading