Skip to content

fix(core): Report actual item count in client reports of envelope send failures #25185

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
Lms24 merged 2 commits into from
Oct 9, 2026
+87 −25
Merged

fix(core): Report actual item count in client reports of envelope send failures #25185

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
cbfd97e
fix(core): Record actual item counts in client reports for failed env…
Lms24 Oct 8, 2026
3de69e6
also handle ratelimit_backoff
Lms24 Oct 8, 2026
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
21 changes: 16 additions & 5 deletions packages/core/src/transports/base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { DEBUG_BUILD } from '../debug-build';
import type { EventDropReason } from '../types/clientreport';
import type { Envelope, EnvelopeItem } from '../types/envelope';
import type { Envelope, EnvelopeItem, EnvelopeItemType } from '../types/envelope';
import type { Event } from '../types/event';
import type {
InternalBaseTransportOptions,
Transport,
Expand Down Expand Up @@ -36,14 +37,24 @@ export function createTransport(
let rateLimits: RateLimits = {};
const flush = (timeout?: number): PromiseLike<boolean> => buffer.drain(timeout);

function recordItemLoss(reason: EventDropReason, [headers, payload]: EnvelopeItem, type: EnvelopeItemType): void {
const dataCategory = envelopeItemTypeToDataCategory(type);
if (type === 'transaction') {
options.recordDroppedEvent(reason, dataCategory);
// The transaction itself counts as a span, too
options.recordDroppedEvent(reason, 'span', ((payload as Event).spans?.length ?? 0) + 1);
} else {
options.recordDroppedEvent(reason, dataCategory, typeof headers.item_count === 'number' ? headers.item_count : 1);
}
}

function send(envelope: Envelope): PromiseLike<TransportMakeRequestResponse> {
const filteredEnvelopeItems: EnvelopeItem[] = [];

// Drop rate limited items from envelope
forEachEnvelopeItem(envelope, (item, type) => {
const dataCategory = envelopeItemTypeToDataCategory(type);
if (isRateLimited(rateLimits, dataCategory)) {
options.recordDroppedEvent('ratelimit_backoff', dataCategory);
if (isRateLimited(rateLimits, envelopeItemTypeToDataCategory(type))) {
recordItemLoss('ratelimit_backoff', item, type);
} else {
filteredEnvelopeItems.push(item);
}
Expand All @@ -64,7 +75,7 @@ export function createTransport(
return;
}
forEachEnvelopeItem(filteredEnvelope, (item, type) => {
options.recordDroppedEvent(reason, envelopeItemTypeToDataCategory(type));
recordItemLoss(reason, item, type);
});
};

Expand Down
91 changes: 71 additions & 20 deletions packages/core/test/lib/transports/base.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { describe, expect, it, vi } from 'vitest';
import { createTransport } from '../../../src/transports/base';
import type { ClientReport } from '../../../src/types/clientreport';
import type { AttachmentItem, EventEnvelope, EventItem } from '../../../src/types/envelope';
import type { AttachmentItem, Envelope, EventEnvelope, EventItem } from '../../../src/types/envelope';
import type { TransportMakeRequestResponse } from '../../../src/types/transport';
import { createClientReportEnvelope } from '../../../src/utils/clientreport';
import { createEnvelope, serializeEnvelope } from '../../../src/utils/envelope';
Expand Down Expand Up @@ -157,7 +157,7 @@ describe('createTransport', () => {

await transport.send(ERROR_ENVELOPE);
expect(requestExecutor).not.toHaveBeenCalled();
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error');
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error', 1);
requestExecutor.mockClear();
recordDroppedEventCallback.mockClear();

Expand Down Expand Up @@ -199,7 +199,7 @@ describe('createTransport', () => {

await transport.send(ERROR_ENVELOPE); // Error envelope should not be sent because of pending rate limit
expect(requestExecutor).not.toHaveBeenCalled();
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error');
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error', 1);
requestExecutor.mockClear();
recordDroppedEventCallback.mockClear();

Expand Down Expand Up @@ -242,18 +242,19 @@ describe('createTransport', () => {
await transport.send(TRANSACTION_ENVELOPE); // Transaction envelope should not be sent because of pending rate limit
expect(requestExecutor).not.toHaveBeenCalled();
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'transaction');
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'span', 1);
requestExecutor.mockClear();
recordDroppedEventCallback.mockClear();

await transport.send(ERROR_ENVELOPE); // Error envelope should not be sent because of pending rate limit
expect(requestExecutor).not.toHaveBeenCalled();
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error');
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error', 1);
requestExecutor.mockClear();
recordDroppedEventCallback.mockClear();

await transport.send(ATTACHMENT_ENVELOPE); // Attachment envelope should not be sent because of pending rate limit
expect(requestExecutor).not.toHaveBeenCalled();
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'attachment');
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'attachment', 1);
requestExecutor.mockClear();
recordDroppedEventCallback.mockClear();

Expand Down Expand Up @@ -302,12 +303,13 @@ describe('createTransport', () => {
await transport.send(TRANSACTION_ENVELOPE); // Transaction envelope should not be sent because of pending rate limit
expect(requestExecutor).not.toHaveBeenCalled();
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'transaction');
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'span', 1);
requestExecutor.mockClear();
recordDroppedEventCallback.mockClear();

await transport.send(ERROR_ENVELOPE); // Error envelope should not be sent because of pending rate limit
expect(requestExecutor).not.toHaveBeenCalled();
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error');
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'error', 1);
requestExecutor.mockClear();
recordDroppedEventCallback.mockClear();

Expand All @@ -324,10 +326,35 @@ describe('createTransport', () => {
expect(requestExecutor).toHaveBeenCalledTimes(1);
expect(recordDroppedEventCallback).not.toHaveBeenCalled();
});

it('records the item count of rate limited container items', async () => {
const { retryAfterSeconds, beforeLimit, withinLimit } = setRateLimitTimes();
const [transport, , requestExecutor, recordDroppedEventCallback] = createTestTransport({
headers: {
'x-sentry-rate-limits': `${retryAfterSeconds}:log_item:scope`,
'retry-after': null,
},
});

const dateNowSpy = vi.spyOn(Date, 'now').mockImplementation(() => beforeLimit);

await transport.send(ERROR_ENVELOPE);
requestExecutor.mockClear();

dateNowSpy.mockImplementation(() => withinLimit);

await transport.send(
createEnvelope<Envelope>({ sent_at: '123' }, [
[{ type: 'log', item_count: 3, content_type: 'application/vnd.sentry.items.log+json' }, { items: [] }],
] as Envelope[1]),
);
expect(requestExecutor).not.toHaveBeenCalled();
expect(recordDroppedEventCallback).toHaveBeenCalledWith('ratelimit_backoff', 'log_item', 3);
});
});

describe('Client Reports', () => {
it('should not record outcomes when client reports fail to send', async () => {
it('does not record outcomes when client reports fail to send', async () => {
expect.assertions(2);

const mockRecordDroppedEventCallback = vi.fn();
Expand All @@ -347,7 +374,7 @@ describe('createTransport', () => {
expect(mockRecordDroppedEventCallback).not.toHaveBeenCalled();
});

it('should not record outcomes when client reports fail due to buffer overflow', async () => {
it('does not record outcomes when client reports fail due to buffer overflow', async () => {
expect.assertions(2);

const mockRecordDroppedEventCallback = vi.fn();
Expand All @@ -371,7 +398,7 @@ describe('createTransport', () => {
expect(mockRecordDroppedEventCallback).not.toHaveBeenCalled();
});

it('should record outcomes when regular events fail to send', async () => {
it('records outcomes when regular events fail to send', async () => {
expect.assertions(2);

const mockRecordDroppedEventCallback = vi.fn();
Expand All @@ -388,12 +415,12 @@ describe('createTransport', () => {
}

// recordDroppedEvent SHOULD be called for regular events
expect(mockRecordDroppedEventCallback).toHaveBeenCalledWith('network_error', 'error');
expect(mockRecordDroppedEventCallback).toHaveBeenCalledWith('network_error', 'error', 1);
});
});

describe('HTTP 413 Content Too Large', () => {
it('should record send_error outcome when receiving 413 response', async () => {
it('records send_error outcome when receiving 413 response', async () => {
const mockRecordDroppedEventCallback = vi.fn();

const transport = createTransport({ recordDroppedEvent: mockRecordDroppedEventCallback }, () =>
Expand All @@ -405,17 +432,17 @@ describe('createTransport', () => {
// Should resolve without throwing
expect(result).toEqual({ statusCode: 413 });
// recordDroppedEvent SHOULD be called with send_error reason
expect(mockRecordDroppedEventCallback).toHaveBeenCalledWith('send_error', 'error');
expect(mockRecordDroppedEventCallback).toHaveBeenCalledWith('send_error', 'error', 1);
});

it('should record send_error for each item in envelope when receiving 413', async () => {
it('records send_error for each item in envelope when receiving 413', async () => {
const mockRecordDroppedEventCallback = vi.fn();

const multiItemEnvelope = createEnvelope<EventEnvelope>(
{ event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2', sent_at: '123' },
[
[{ type: 'event' }, { event_id: 'aa3ff046696b4bc6b609ce6d28fde9e2' }] as EventItem,
[{ type: 'transaction' }, { event_id: 'bb3ff046696b4bc6b609ce6d28fde9e2' }] as EventItem,
[{ type: 'transaction' }, { event_id: 'bb3ff046696b4bc6b609ce6d28fde9e2', spans: [{}, {}] }] as EventItem,
],
);

Expand All @@ -425,13 +452,37 @@ describe('createTransport', () => {

await transport.send(multiItemEnvelope);

// recordDroppedEvent SHOULD be called for each item
expect(mockRecordDroppedEventCallback).toHaveBeenCalledTimes(2);
expect(mockRecordDroppedEventCallback).toHaveBeenCalledWith('send_error', 'error');
expect(mockRecordDroppedEventCallback).toHaveBeenCalledTimes(3);
expect(mockRecordDroppedEventCallback).toHaveBeenCalledWith('send_error', 'error', 1);
expect(mockRecordDroppedEventCallback).toHaveBeenCalledWith('send_error', 'transaction');
expect(mockRecordDroppedEventCallback).toHaveBeenCalledWith('send_error', 'span', 3);
});

it('records send_error with the item count of container items when receiving 413', async () => {
const mockRecordDroppedEventCallback = vi.fn();

const containerEnvelope = createEnvelope<Envelope>({ sent_at: '123' }, [
[{ type: 'span', item_count: 5, content_type: 'application/vnd.sentry.items.span.v2+json' }, { items: [] }],
[{ type: 'log', item_count: 3, content_type: 'application/vnd.sentry.items.log+json' }, { items: [] }],
[
{ type: 'trace_metric', item_count: 2, content_type: 'application/vnd.sentry.items.trace-metric+json' },
{ items: [] },
],
] as Envelope[1]);

const transport = createTransport({ recordDroppedEvent: mockRecordDroppedEventCallback }, () =>
resolvedSyncPromise({ statusCode: 413 }),
);

await transport.send(containerEnvelope);

expect(mockRecordDroppedEventCallback).toHaveBeenCalledTimes(3);
expect(mockRecordDroppedEventCallback).toHaveBeenCalledWith('send_error', 'span', 5);
expect(mockRecordDroppedEventCallback).toHaveBeenCalledWith('send_error', 'log_item', 3);
expect(mockRecordDroppedEventCallback).toHaveBeenCalledWith('send_error', 'metric', 2);
});

it('should not record outcomes for client reports when receiving 413', async () => {
it('does not record outcomes for client reports when receiving 413', async () => {
const mockRecordDroppedEventCallback = vi.fn();

const transport = createTransport({ recordDroppedEvent: mockRecordDroppedEventCallback }, () =>
Expand All @@ -446,7 +497,7 @@ describe('createTransport', () => {
expect(mockRecordDroppedEventCallback).not.toHaveBeenCalled();
});

it('should not apply rate limits after receiving 413', async () => {
it('does not apply rate limits after receiving 413', async () => {
const mockRecordDroppedEventCallback = vi.fn();
const mockRequestExecutor = vi.fn(() => resolvedSyncPromise({ statusCode: 413 }));

Expand All @@ -455,7 +506,7 @@ describe('createTransport', () => {
// First request gets 413
await transport.send(ERROR_ENVELOPE);
expect(mockRequestExecutor).toHaveBeenCalledTimes(1);
expect(mockRecordDroppedEventCallback).toHaveBeenCalledWith('send_error', 'error');
expect(mockRecordDroppedEventCallback).toHaveBeenCalledWith('send_error', 'error', 1);
mockRequestExecutor.mockClear();
mockRecordDroppedEventCallback.mockClear();

Expand Down
Loading