-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(effect): Add metrics to Sentry.effectLayer #19709
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.
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
Changes from all commits
10e78a5
98fe46f
5b1c7d8
6fa5fa8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| import { metrics as sentryMetrics } from '@sentry/core'; | ||
| import * as Effect from 'effect/Effect'; | ||
| import type * as Layer from 'effect/Layer'; | ||
| import { scopedDiscard } from 'effect/Layer'; | ||
| import * as Metric from 'effect/Metric'; | ||
| import * as MetricKeyType from 'effect/MetricKeyType'; | ||
| import type * as MetricPair from 'effect/MetricPair'; | ||
| import * as MetricState from 'effect/MetricState'; | ||
| import * as Schedule from 'effect/Schedule'; | ||
|
|
||
| type MetricAttributes = Record<string, string>; | ||
|
|
||
| function labelsToAttributes(labels: ReadonlyArray<{ key: string; value: string }>): MetricAttributes { | ||
| return labels.reduce((acc, label) => ({ ...acc, [label.key]: label.value }), {}); | ||
| } | ||
|
|
||
| function sendMetricToSentry(pair: MetricPair.MetricPair.Untyped): void { | ||
| const { metricKey, metricState } = pair; | ||
| const name = metricKey.name; | ||
| const attributes = labelsToAttributes(metricKey.tags); | ||
|
|
||
| if (MetricState.isCounterState(metricState)) { | ||
| const value = Number(metricState.count); | ||
| sentryMetrics.count(name, value, { attributes }); | ||
| } else if (MetricState.isGaugeState(metricState)) { | ||
| const value = Number(metricState.value); | ||
| sentryMetrics.gauge(name, value, { attributes }); | ||
| } else if (MetricState.isHistogramState(metricState)) { | ||
| sentryMetrics.gauge(`${name}.sum`, metricState.sum, { attributes }); | ||
| sentryMetrics.gauge(`${name}.count`, metricState.count, { attributes }); | ||
| sentryMetrics.gauge(`${name}.min`, metricState.min, { attributes }); | ||
| sentryMetrics.gauge(`${name}.max`, metricState.max, { attributes }); | ||
| } else if (MetricState.isSummaryState(metricState)) { | ||
| sentryMetrics.gauge(`${name}.sum`, metricState.sum, { attributes }); | ||
| sentryMetrics.gauge(`${name}.count`, metricState.count, { attributes }); | ||
| sentryMetrics.gauge(`${name}.min`, metricState.min, { attributes }); | ||
| sentryMetrics.gauge(`${name}.max`, metricState.max, { attributes }); | ||
| } else if (MetricState.isFrequencyState(metricState)) { | ||
| for (const [word, count] of metricState.occurrences) { | ||
| sentryMetrics.count(name, count, { | ||
| attributes: { ...attributes, word }, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| function getMetricId(pair: MetricPair.MetricPair.Untyped): string { | ||
| const tags = pair.metricKey.tags.map(t => `${t.key}=${t.value}`).join(','); | ||
| return `${pair.metricKey.name}:${tags}`; | ||
| } | ||
|
|
||
| function sendDeltaMetricToSentry( | ||
| pair: MetricPair.MetricPair.Untyped, | ||
| previousCounterValues: Map<string, number>, | ||
| ): void { | ||
| const { metricKey, metricState } = pair; | ||
| const name = metricKey.name; | ||
| const attributes = labelsToAttributes(metricKey.tags); | ||
| const metricId = getMetricId(pair); | ||
|
|
||
| if (MetricState.isCounterState(metricState)) { | ||
| const currentValue = Number(metricState.count); | ||
|
|
||
| const previousValue = previousCounterValues.get(metricId) ?? 0; | ||
| const delta = currentValue - previousValue; | ||
|
|
||
| if (delta > 0) { | ||
| sentryMetrics.count(name, delta, { attributes }); | ||
| } | ||
|
|
||
| previousCounterValues.set(metricId, currentValue); | ||
| } else { | ||
| sendMetricToSentry(pair); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Flushes all Effect metrics to Sentry. | ||
| * @param previousCounterValues - Map tracking previous counter values for delta calculation | ||
| */ | ||
| function flushMetricsToSentry(previousCounterValues: Map<string, number>): void { | ||
| const snapshot = Metric.unsafeSnapshot(); | ||
|
|
||
| snapshot.forEach((pair: MetricPair.MetricPair.Untyped) => { | ||
| if (MetricKeyType.isCounterKey(pair.metricKey.keyType)) { | ||
| sendDeltaMetricToSentry(pair, previousCounterValues); | ||
| } else { | ||
| sendMetricToSentry(pair); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Creates a metrics flusher with its own isolated state for delta tracking. | ||
| * Useful for testing scenarios where you need to control the lifecycle. | ||
| * @internal | ||
| */ | ||
| export function createMetricsFlusher(): { | ||
| flush: () => void; | ||
| clear: () => void; | ||
| } { | ||
| const previousCounterValues = new Map<string, number>(); | ||
| return { | ||
| flush: () => flushMetricsToSentry(previousCounterValues), | ||
| clear: () => previousCounterValues.clear(), | ||
| }; | ||
| } | ||
|
|
||
| function createMetricsReporterEffect(previousCounterValues: Map<string, number>): Effect.Effect<void, never, never> { | ||
| const schedule = Schedule.spaced('10 seconds'); | ||
|
|
||
| return Effect.repeat( | ||
| Effect.sync(() => flushMetricsToSentry(previousCounterValues)), | ||
| schedule, | ||
| ).pipe(Effect.asVoid, Effect.interruptible); | ||
| } | ||
|
|
||
| /** | ||
| * Effect Layer that periodically flushes metrics to Sentry. | ||
| * The layer manages its own state for delta counter calculations, | ||
| * which is automatically cleaned up when the layer is finalized. | ||
| */ | ||
| export const SentryEffectMetricsLayer: Layer.Layer<never, never, never> = scopedDiscard( | ||
| Effect.gen(function* () { | ||
| const previousCounterValues = new Map<string, number>(); | ||
|
|
||
| yield* Effect.acquireRelease(Effect.void, () => | ||
| Effect.sync(() => { | ||
| previousCounterValues.clear(); | ||
| }), | ||
| ); | ||
|
|
||
| yield* Effect.forkScoped(createMetricsReporterEffect(previousCounterValues)); | ||
| }), | ||
| ); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No final metrics flush before layer teardownMedium Severity The |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,8 +48,22 @@ describe('buildEffectLayer', () => { | |
| expect(Layer.isLayer(layer)).toBe(true); | ||
| }); | ||
|
|
||
| it('returns a valid layer with enableMetrics: false', () => { | ||
| const layer = buildEffectLayer({ enableMetrics: false }, mockClient); | ||
|
|
||
| expect(layer).toBeDefined(); | ||
| expect(Layer.isLayer(layer)).toBe(true); | ||
| }); | ||
|
|
||
| it('returns a valid layer with enableMetrics: true', () => { | ||
| const layer = buildEffectLayer({ enableMetrics: true }, mockClient); | ||
|
|
||
| expect(layer).toBeDefined(); | ||
| expect(Layer.isLayer(layer)).toBe(true); | ||
| }); | ||
|
|
||
| it('returns a valid layer with all features enabled', () => { | ||
| const layer = buildEffectLayer({ enableLogs: true }, mockClient); | ||
| const layer = buildEffectLayer({ enableLogs: true, enableMetrics: true }, mockClient); | ||
|
|
||
| expect(layer).toBeDefined(); | ||
| expect(Layer.isLayer(layer)).toBe(true); | ||
|
|
@@ -71,20 +85,18 @@ describe('buildEffectLayer', () => { | |
| }).pipe(Effect.provide(buildEffectLayer({ enableLogs: true }, mockClient))), | ||
| ); | ||
|
|
||
| it.effect('layer with logs disabled routes Effect does not log to Sentry logger', () => | ||
| Effect.gen(function* () { | ||
| const infoSpy = vi.spyOn(sentryLogger, 'info'); | ||
| yield* Effect.log('test log message'); | ||
| expect(infoSpy).not.toHaveBeenCalled(); | ||
| infoSpy.mockRestore(); | ||
| }).pipe(Effect.provide(buildEffectLayer({ enableLogs: false }, mockClient))), | ||
| ); | ||
| it('returns different layer when enableMetrics is true vs false', () => { | ||
| const layerWithMetrics = buildEffectLayer({ enableMetrics: true }, mockClient); | ||
| const layerWithoutMetrics = buildEffectLayer({ enableMetrics: false }, mockClient); | ||
|
|
||
| expect(layerWithMetrics).not.toBe(layerWithoutMetrics); | ||
| }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed test for disabled logs, no metrics equivalentLow 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 Triggered by project rule: PR Review Guidelines for Cursor Bot |
||
|
|
||
| it.effect('layer with all features enabled can be provided to an Effect program', () => | ||
| Effect.gen(function* () { | ||
| const result = yield* Effect.succeed('all-features'); | ||
| expect(result).toBe('all-features'); | ||
| }).pipe(Effect.provide(buildEffectLayer({ enableLogs: true }, mockClient))), | ||
| }).pipe(Effect.provide(buildEffectLayer({ enableLogs: true, enableMetrics: true }, mockClient))), | ||
| ); | ||
|
|
||
| it.effect('layer enables tracing for Effect spans via Sentry tracer', () => | ||
|
|
@@ -109,9 +121,10 @@ describe('buildEffectLayer', () => { | |
| const layer = buildEffectLayer( | ||
| { | ||
| enableLogs: true, | ||
| enableMetrics: true, | ||
| dsn: 'https://test@sentry.io/123', | ||
| debug: true, | ||
| } as { enableLogs?: boolean; dsn?: string; debug?: boolean }, | ||
| } as { enableLogs?: boolean; enableMetrics?: boolean; dsn?: string; debug?: boolean }, | ||
| mockClient, | ||
| ); | ||
|
|
||
|
|
||


There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 usesendDeltaMetricToSentryfor this, but frequency metrics go throughsendMetricToSentrywithout any delta logic.Additional Locations (1)
packages/effect/src/metrics.ts#L84-L91