feat(effect): Add metrics to Sentry.effectLayer#19709
feat(effect): Add metrics to Sentry.effectLayer#19709JPeer264 wants to merge 3 commits intojp/add-effect-sdkfrom
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| attributes: { ...attributes, word }, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Frequency metrics double-counted on every flush cycle
High Severity
Frequency metrics use sentryMetrics.count() (which is additive) with cumulative occurrence counts from Effect's state, but lack delta tracking. Every 10-second flush re-sends the full cumulative counts, causing Sentry to multiply the actual values by the number of flushes. Counters correctly use sendDeltaMetricToSentry for this, but frequency metrics go through sendMetricToSentry without any delta logic.
Additional Locations (1)
| const value = typeof metricState.value === 'bigint' ? Number(metricState.value) : metricState.value; | ||
| sentryMetrics.gauge(name, value, { attributes }); | ||
| } else if (MetricState.isHistogramState(metricState)) { | ||
| sentryMetrics.distribution(`${name}.sum`, metricState.sum, { attributes }); |
There was a problem hiding this comment.
Histogram/summary cumulative sum re-emitted as new distribution points
Medium Severity
For histogram and summary states, metricState.sum (a cumulative value) is emitted via sentryMetrics.distribution() on each flush. Since distribution records a new data point per call, the cumulative sum gets repeatedly added as a fresh observation every 10 seconds, producing incorrect distribution data in Sentry. This likely needs to be a gauge or use delta tracking.
Additional Locations (1)
| expect(mockCount).toHaveBeenCalledWith('flush_test_delta_counter', 5, { attributes: {} }); | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
No integration or E2E test for feat PR
Low Severity
This is a feat PR but only includes unit tests. Per project review rules, feat PRs are expected to include at least one integration or E2E test. Consider adding an integration test that exercises the full metrics flush pipeline with a real (or realistic) Sentry transport to verify end-to-end behavior.
Triggered by project rule: PR Review Guidelines for Cursor Bot
size-limit report 📦
|
| } | ||
|
|
||
| const { enableLogs = false } = options; | ||
| const { enableLogs = false, enableMetrics = false } = options; |
There was a problem hiding this comment.
q: Our Metrics need to default to true (https://develop.sentry.dev/sdk/telemetry/metrics/#initialization-options). Why do we override this here?
There was a problem hiding this comment.
TIL. Thanks for giving the hint here - I assumed it is the same default as enableLogs, I'll tripple check if it would be possible to not even touch that at all and rely on the options entirely.
There was a problem hiding this comment.
Alright. I'll redo the buildEffectLayer in a follow up PR, so the options are coming from the client directly - so we don't have to think about updating these here once the defaults are changing. For now I switched the default to true to make it correct.
There was a problem hiding this comment.
Alright, we discussed offline that we need a opt-in for metrics and logs in a form of a different naming. I'd like to keep the settings as is in this PR and focus on the metrics implementation itself. On a follow-up PR I'd like to propose the new settings (this PR is anyways onto jp/add-effect-sdk, so there is nothing lost now)
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: No final metrics flush before layer teardown
- Added a final flush call in the release effect before clearing previousCounterValues to ensure all accumulated metrics are sent to Sentry before the layer is torn down.
Or push these changes by commenting:
@cursor push 80a4dab25a
Preview (80a4dab25a)
diff --git a/packages/effect/src/metrics.ts b/packages/effect/src/metrics.ts
--- a/packages/effect/src/metrics.ts
+++ b/packages/effect/src/metrics.ts
@@ -126,6 +126,7 @@
yield* Effect.acquireRelease(Effect.void, () =>
Effect.sync(() => {
+ flushMetricsToSentry(previousCounterValues);
previousCounterValues.clear();
}),
);|
|
||
| yield* Effect.forkScoped(createMetricsReporterEffect(previousCounterValues)); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
No final metrics flush before layer teardown
Medium Severity
The SentryEffectMetricsLayer release effect clears previousCounterValues but never performs a final flush. When the layer is torn down, the scoped fiber running the periodic reporter is interrupted, and any metrics accumulated since the last 10-second flush are silently lost.
2 participants
This adds metrics to the
Sentry.effectLayer. It is enabled whenenableMetrics: trueis added as option