fix(core): Ensure metrics past MAX_METRIC_BUFFER_SIZE are not swallowed #18212
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| // 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'); |
There was a problem hiding this comment.
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
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
Looks like we swallowed the metric that triggers a flush when MAX_METRIC_BUFFER_SIZE is surpassed.
Test demonstrating issue: f0737fa
Fix: 1a4e02a
Related logs pr: #18207