feat(effect): Add metrics to Sentry.effectLayer#19709
feat(effect): Add metrics to Sentry.effectLayer#19709JPeer264 merged 4 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)
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)
see: #19755
![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.
Autofix Details
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.
packages/effect/src/metrics.ts
Outdated
| const attributes = labelsToAttributes(metricKey.tags); | ||
|
|
||
| if (MetricState.isCounterState(metricState)) { | ||
| const value = typeof metricState.count === 'bigint' ? Number(metricState.count) : metricState.count; |
There was a problem hiding this comment.
ultra-l: I'd just always call Number(metricState.count), no need to do an extra check
packages/effect/src/metrics.ts
Outdated
| const metricId = getMetricId(pair); | ||
|
|
||
| if (MetricState.isCounterState(metricState)) { | ||
| const currentValue = typeof metricState.count === 'bigint' ? Number(metricState.count) : metricState.count; |
There was a problem hiding this comment.
ultra-l: same here, just Number(metricState.count)?
JPeer264
force-pushed
the
jp/add-effect-sdk-stack/5
branch
from
6cc903f to
6fa5fa8
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| const layerWithoutMetrics = buildEffectLayer({ enableMetrics: false }, mockClient); | ||
|
|
||
| expect(layerWithMetrics).not.toBe(layerWithoutMetrics); | ||
| }); |
There was a problem hiding this comment.
Removed test for disabled logs, no metrics equivalent
Low Severity
The existing test verifying that disabled logs don't route to the Sentry logger was removed and replaced with a weaker structural comparison test. There's also no integration or E2E test verifying that the metrics layer actually sends metrics to Sentry through the full effectLayer pipeline. For a feat PR, at least one integration or E2E test is recommended per the review guidelines.
Triggered by project rule: PR Review Guidelines for Cursor Bot
This adds metrics to the `Sentry.effectLayer`. It is enabled when `enableMetrics: true` is added as option
This adds metrics to the `Sentry.effectLayer`. It is enabled when `enableMetrics: true` is added as option
3 participants
This adds metrics to the
Sentry.effectLayer. It is enabled whenenableMetrics: trueis added as option