-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(core): Add a PromiseBuffer for incoming events on the client #18120
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
Changes from all commits
9a8c083
f3a52d7
7fec5cd
63b4be9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ import { | |
| import * as integrationModule from '../../src/integration'; | ||
| import { _INTERNAL_captureLog } from '../../src/logs/internal'; | ||
| import { _INTERNAL_captureMetric } from '../../src/metrics/internal'; | ||
| import { DEFAULT_TRANSPORT_BUFFER_SIZE } from '../../src/transports/base'; | ||
| import type { Envelope } from '../../src/types-hoist/envelope'; | ||
| import type { ErrorEvent, Event, TransactionEvent } from '../../src/types-hoist/event'; | ||
| import type { SpanJSON } from '../../src/types-hoist/span'; | ||
|
|
@@ -23,7 +24,7 @@ import * as miscModule from '../../src/utils/misc'; | |
| import * as stringModule from '../../src/utils/string'; | ||
| import * as timeModule from '../../src/utils/time'; | ||
| import { getDefaultTestClientOptions, TestClient } from '../mocks/client'; | ||
| import { AdHocIntegration, TestIntegration } from '../mocks/integration'; | ||
| import { AdHocIntegration, AsyncTestIntegration, TestIntegration } from '../mocks/integration'; | ||
| import { makeFakeTransport } from '../mocks/transport'; | ||
| import { clearGlobalScope } from '../testutils'; | ||
|
|
||
|
|
@@ -2935,4 +2936,66 @@ describe('Client', () => { | |
| expect(sendEnvelopeSpy).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); | ||
|
|
||
| describe('promise buffer usage', () => { | ||
| it('respects the default value of the buffer size', async () => { | ||
| const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN }); | ||
| const client = new TestClient(options); | ||
|
|
||
| client.addIntegration(new AsyncTestIntegration()); | ||
|
|
||
| Array.from({ length: DEFAULT_TRANSPORT_BUFFER_SIZE + 1 }).forEach(() => { | ||
| client.captureException(new Error('ʕノ•ᴥ•ʔノ ︵ ┻━┻')); | ||
| }); | ||
|
|
||
| expect(client._clearOutcomes()).toEqual([{ reason: 'queue_overflow', category: 'error', quantity: 1 }]); | ||
| }); | ||
|
|
||
| it('records queue_overflow when promise buffer is full', async () => { | ||
| const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, transportOptions: { bufferSize: 1 } }); | ||
| const client = new TestClient(options); | ||
|
|
||
| client.addIntegration(new AsyncTestIntegration()); | ||
|
|
||
| client.captureException(new Error('first')); | ||
| client.captureException(new Error('second')); | ||
| client.captureException(new Error('third')); | ||
|
|
||
| expect(client._clearOutcomes()).toEqual([{ reason: 'queue_overflow', category: 'error', quantity: 2 }]); | ||
| }); | ||
|
|
||
| it('records different types of dropped events', async () => { | ||
| const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, transportOptions: { bufferSize: 1 } }); | ||
| const client = new TestClient(options); | ||
|
|
||
| client.addIntegration(new AsyncTestIntegration()); | ||
|
|
||
| client.captureException(new Error('first')); // error | ||
| client.captureException(new Error('second')); // error | ||
| client.captureMessage('third'); // unknown | ||
| client.captureEvent({ message: 'fourth' }); // error | ||
| client.captureEvent({ message: 'fifth', type: 'replay_event' }); // replay | ||
| client.captureEvent({ message: 'sixth', type: 'transaction' }); // transaction | ||
|
|
||
| expect(client._clearOutcomes()).toEqual([ | ||
| { reason: 'queue_overflow', category: 'error', quantity: 2 }, | ||
| { reason: 'queue_overflow', category: 'unknown', quantity: 1 }, | ||
| { reason: 'queue_overflow', category: 'replay', quantity: 1 }, | ||
| { reason: 'queue_overflow', category: 'transaction', quantity: 1 }, | ||
| ]); | ||
| }); | ||
|
|
||
| it('should skip the promise buffer with sync integrations', async () => { | ||
| const options = getDefaultTestClientOptions({ dsn: PUBLIC_DSN, transportOptions: { bufferSize: 1 } }); | ||
| const client = new TestClient(options); | ||
|
|
||
| client.addIntegration(new TestIntegration()); | ||
|
|
||
| client.captureException(new Error('first')); | ||
| client.captureException(new Error('second')); | ||
| client.captureException(new Error('third')); | ||
|
|
||
| expect(client._clearOutcomes()).toEqual([]); | ||
| }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Test expects wrong outcome for sync integrationsThe test expects no dropped events when calling |
||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,16 @@ export class TestIntegration implements Integration { | |
| } | ||
| } | ||
|
|
||
| export class AsyncTestIntegration implements Integration { | ||
| public static id: string = 'AsyncTestIntegration'; | ||
|
|
||
| public name: string = 'AsyncTestIntegration'; | ||
|
|
||
| processEvent(event: Event): Event | null | PromiseLike<Event | null> { | ||
| return new Promise(resolve => setTimeout(() => resolve(event), 1)); | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Async Test Integration Missing Event Processor SetupThe |
||
|
|
||
| export class AddAttachmentTestIntegration implements Integration { | ||
| public static id: string = 'AddAttachmentTestIntegration'; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Promise created eagerly in captureMessage
In
captureMessage, thepromisedEventis created outside the task producer function passed to_process. This meanseventFromMessageoreventFromExceptionis called immediately, even when the promise buffer is full. This defeats the lazy evaluation design of the promise buffer, causing unnecessary work when events should be dropped. The promise creation should be moved inside the task producer function to enable proper lazy evaluation.