Skip to content

fix(core): Ensure metrics past MAX_METRIC_BUFFER_SIZE are not swallowed #18212

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 3 commits into from
Nov 17, 2025
Merged

fix(core): Ensure metrics past MAX_METRIC_BUFFER_SIZE are not swallowed #18212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d1ea9e9
fix(core): Ensure metrics past MAX_METRIC_BUFFER_SIZE are not swallowed
andreiborza Nov 14, 2025
f0737fa
Add test
andreiborza Nov 14, 2025
1a4e02a
Ensure metric that triggers flush on MAX_METRIC_BUFFER_SIZE is not sw…
andreiborza Nov 14, 2025
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
6 changes: 4 additions & 2 deletions packages/core/src/metrics/internal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,14 +88,16 @@ function setMetricAttribute(
*/
export function _INTERNAL_captureSerializedMetric(client: Client, serializedMetric: SerializedMetric): void {
const bufferMap = _getBufferMap();

const metricBuffer = _INTERNAL_getMetricBuffer(client);

if (metricBuffer === undefined) {
bufferMap.set(client, [serializedMetric]);
} else {
bufferMap.set(client, [...metricBuffer, serializedMetric]);
if (metricBuffer.length >= MAX_METRIC_BUFFER_SIZE) {
_INTERNAL_flushMetricsBuffer(client, metricBuffer);
bufferMap.set(client, [serializedMetric]);
} else {
bufferMap.set(client, [...metricBuffer, serializedMetric]);
}
}
}
Expand Down
5 changes: 4 additions & 1 deletion packages/core/test/lib/metrics/internal.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,10 @@ describe('_INTERNAL_captureMetric', () => {
// Add one more to trigger flush
_INTERNAL_captureMetric({ type: 'counter', name: 'trigger.flush', value: 999 }, { scope });

expect(_INTERNAL_getMetricBuffer(client)).toEqual([]);
// After flushing the 1000 metrics, the new metric starts a fresh buffer with 1 item
const buffer = _INTERNAL_getMetricBuffer(client);
expect(buffer).toHaveLength(1);
expect(buffer?.[0]?.name).toBe('trigger.flush');
Comment on lines +259 to +262
Copy link

@sentry sentry bot Nov 14, 2025

Choose a reason for hiding this comment

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

Bug: A metric that triggers a buffer flush is lost because the flush operation clears the buffer after the new metric has been added but before it's processed.
Severity: CRITICAL | Confidence: 1.00

🔍 Detailed Analysis

The _INTERNAL_captureSerializedMetric function incorrectly handles metrics when the buffer reaches MAX_METRIC_BUFFER_SIZE. When a new metric arrives, it's added to the buffer via bufferMap.set(client, [...metricBuffer, serializedMetric]). However, the subsequent flush condition check if (metricBuffer.length >= MAX_METRIC_BUFFER_SIZE) and the flush operation _INTERNAL_flushMetricsBuffer(client, metricBuffer) use the old metricBuffer reference (before the new metric was added). This leads to the newly added metric being present in the map when _INTERNAL_flushMetricsBuffer is called, but then being lost when _getBufferMap().set(client, []) clears the entire buffer, including the new metric. This results in data loss for metrics that trigger a buffer flush.

💡 Suggested Fix

Modify _INTERNAL_captureSerializedMetric to ensure that the _INTERNAL_flushMetricsBuffer function is called with the updated buffer, or that the new metric is handled correctly after the old buffer is flushed and cleared, perhaps by re-adding it to a newly cleared buffer.

🤖 Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: packages/core/test/lib/metrics/internal.test.ts#L259-L262

Potential issue: The `_INTERNAL_captureSerializedMetric` function incorrectly handles
metrics when the buffer reaches `MAX_METRIC_BUFFER_SIZE`. When a new metric arrives,
it's added to the buffer via `bufferMap.set(client, [...metricBuffer,
serializedMetric])`. However, the subsequent flush condition check `if
(metricBuffer.length >= MAX_METRIC_BUFFER_SIZE)` and the flush operation
`_INTERNAL_flushMetricsBuffer(client, metricBuffer)` use the *old* `metricBuffer`
reference (before the new metric was added). This leads to the newly added metric being
present in the map when `_INTERNAL_flushMetricsBuffer` is called, but then being lost
when `_getBufferMap().set(client, [])` clears the entire buffer, including the new
metric. This results in data loss for metrics that trigger a buffer flush.

Did we get this right? 👍 / 👎 to inform future reviews.

Reference_id: 2692398

Copy link
Member Author

@andreiborza andreiborza Nov 14, 2025

Choose a reason for hiding this comment

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

That's what we are fixing with 1a4e02a

});

it('does not flush metrics buffer when it is empty', () => {
Expand Down
Loading