feat(core): Decouple metrics aggregation from client (#10628)

This is a re-opening of #10596 which was closed automatically after the
branch is was based off was closed.

Part of the work for #10595

This PR decouples metrics aggregation from the client and stores the
aggregator globally the first time it is required.

This PR does not remove `captureAggregateMetrics` from the `BaseClient`
or make `sendEnvelope` public as this can occur in a later PR. This will
remove the remaining metrics code from the most basic tree-shaken Sentry
configuration.

The default (node) metrics implementation remains in `@sentry/core`
because it's required for Deno and Vercel-Edge which don't ref.
`@sentry/node`.

There's a load of extra deletions and cleanup that can occur for v8 when
the integration is deleted. For example `BrowserMetricsAggregator` can
probably move to `@sentry/browser`.

Author: timfish

MIGRATION.md

@@ -1,5 +1,9 @@
 # Upgrading from 7.x to 8.x
 
+## Removal of the `MetricsAggregator` integration class and `metricsAggregatorIntegration`
+
+The SDKs now support metrics features without any additional configuration.
+
 ## Updated behaviour of `tracePropagationTargets` in the browser (HTTP tracing headers & CORS)
 
 We updated the behaviour of the SDKs when no `tracePropagationTargets` option was defined. As a reminder, you can

dev-packages/browser-integration-tests/suites/metrics/init.js

@@ -0,0 +1,16 @@
+import * as Sentry from '@sentry/browser';
+
+window.Sentry = Sentry;
+
+Sentry.init({
+  dsn: 'https://[email protected]/1337',
+});
+
+Sentry.metrics.increment('increment');
+Sentry.metrics.increment('increment');
+Sentry.metrics.distribution('distribution', 42);
+Sentry.metrics.distribution('distribution', 45);
+Sentry.metrics.gauge('gauge', 5);
+Sentry.metrics.gauge('gauge', 15);
+Sentry.metrics.set('set', 'nope');
+Sentry.metrics.set('set', 'another');

dev-packages/browser-integration-tests/suites/metrics/test.ts

@@ -0,0 +1,17 @@
+import { expect } from '@playwright/test';
+
+import { sentryTest } from '../../utils/fixtures';
+import { getFirstSentryEnvelopeRequest, properEnvelopeRequestParser } from '../../utils/helpers';
+
+sentryTest('collects metrics', async ({ getLocalTestUrl, page }) => {
+  const url = await getLocalTestUrl({ testDir: __dirname });
+
+  const statsdBuffer = await getFirstSentryEnvelopeRequest<Uint8Array>(page, url, properEnvelopeRequestParser);
+  const statsdString = new TextDecoder().decode(statsdBuffer);
+  // Replace all the Txxxxxx to remove the timestamps
+  const normalisedStatsdString = statsdString.replace(/T\d+\n?/g, 'T000000');
+
+  expect(normalisedStatsdString).toEqual(
+    'increment@none:2|c|T000000distribution@none:42:45|d|T000000gauge@none:15:5:15:20:2|g|T000000set@none:3387254:3443787523|s|T000000',
+  );
+});

dev-packages/browser-integration-tests/utils/helpers.ts

@@ -1,5 +1,6 @@
 import type { Page, Request } from '@playwright/test';
-import type { EnvelopeItemType, Event, EventEnvelopeHeaders } from '@sentry/types';
+import type { EnvelopeItem, EnvelopeItemType, Event, EventEnvelopeHeaders } from '@sentry/types';
+import { parseEnvelope } from '@sentry/utils';
 
 export const envelopeUrlRegex = /\.sentry\.io\/api\/\d+\/envelope\//;
 
@@ -21,6 +22,27 @@ export const envelopeRequestParser = <T = Event>(request: Request | null, envelo
   return envelopeParser(request)[envelopeIndex] as T;
 };
 
+/**
+ * The above envelope parser does not follow the envelope spec...
+ * ...but modifying it to follow the spec breaks a lot of the test which rely on the current indexing behavior.
+ *
+ * This parser is a temporary solution to allow us to test metrics with statsd envelopes.
+ *
+ * Eventually, all the tests should be migrated to use this 'proper' envelope parser!
+ */
+export const properEnvelopeParser = (request: Request | null): EnvelopeItem[] => {
+  // https://develop.sentry.dev/sdk/envelopes/
+  const envelope = request?.postData() || '';
+
+  const [, items] = parseEnvelope(envelope, new TextEncoder(), new TextDecoder());
+
+  return items;
+};
+
+export const properEnvelopeRequestParser = <T = Event>(request: Request | null, envelopeIndex = 1): T => {
+  return properEnvelopeParser(request)[0][envelopeIndex] as T;
+};
+
 export const envelopeHeaderRequestParser = (request: Request | null): EventEnvelopeHeaders => {
   // https://develop.sentry.dev/sdk/envelopes/
   const envelope = request?.postData() || '';

dev-packages/node-integration-tests/suites/tracing/metric-summaries/scenario.js

@@ -6,9 +6,6 @@ Sentry.init({
   release: '1.0',
   tracesSampleRate: 1.0,
   transport: loggingTransport,
-  _experiments: {
-    metricsAggregator: true,
-  },
 });
 
 // Stop the process from exiting before the transaction is sent

packages/astro/src/index.types.ts

@@ -26,4 +26,5 @@ export declare const defaultStackParser: StackParser;
 export declare function close(timeout?: number | undefined): PromiseLike<boolean>;
 export declare function flush(timeout?: number | undefined): PromiseLike<boolean>;
 
+export declare const metrics: typeof clientSdk.metrics & typeof serverSdk.metrics;
 export default sentryAstro;

packages/browser/src/exports.ts

@@ -64,7 +64,6 @@ export {
   FunctionToString,
   // eslint-disable-next-line deprecation/deprecation
   InboundFilters,
-  metrics,
   functionToStringIntegration,
   inboundFiltersIntegration,
   parameterize,
@@ -77,6 +76,8 @@ export {
   SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE,
 } from '@sentry/core';
 
+export * from './metrics';
+
 export { WINDOW } from './helpers';
 export { BrowserClient } from './client';
 export { makeFetchTransport, makeXHRTransport } from './transports';

packages/browser/src/metrics.ts

@@ -0,0 +1,45 @@
+import type { MetricData } from '@sentry/core';
+import { BrowserMetricsAggregator, metrics as metricsCore } from '@sentry/core';
+
+/**
+ * Adds a value to a counter metric
+ *
+ * @experimental This API is experimental and might have breaking changes in the future.
+ */
+function increment(name: string, value: number = 1, data?: MetricData): void {
+  metricsCore.increment(BrowserMetricsAggregator, name, value, data);
+}
+
+/**
+ * Adds a value to a distribution metric
+ *
+ * @experimental This API is experimental and might have breaking changes in the future.
+ */
+function distribution(name: string, value: number, data?: MetricData): void {
+  metricsCore.distribution(BrowserMetricsAggregator, name, value, data);
+}
+
+/**
+ * Adds a value to a set metric. Value must be a string or integer.
+ *
+ * @experimental This API is experimental and might have breaking changes in the future.
+ */
+function set(name: string, value: number | string, data?: MetricData): void {
+  metricsCore.set(BrowserMetricsAggregator, name, value, data);
+}
+
+/**
+ * Adds a value to a gauge metric
+ *
+ * @experimental This API is experimental and might have breaking changes in the future.
+ */
+function gauge(name: string, value: number, data?: MetricData): void {
+  metricsCore.gauge(BrowserMetricsAggregator, name, value, data);
+}
+
+export const metrics = {
+  increment,
+  distribution,
+  set,
+  gauge,
+};

packages/bun/src/index.ts

@@ -71,7 +71,7 @@ export {
   startInactiveSpan,
   startSpanManual,
   continueTrace,
-  metrics,
+  metricsDefault as metrics,
   functionToStringIntegration,
   inboundFiltersIntegration,
   linkedErrorsIntegration,

packages/core/src/baseclient.ts

@@ -17,7 +17,6 @@ import type {
   Integration,
   IntegrationClass,
   MetricBucketItem,
-  MetricsAggregator,
   Outcome,
   ParameterizedString,
   SdkMetadata,
@@ -93,13 +92,6 @@ const ALREADY_SEEN_ERROR = "Not capturing exception because it's already been ca
  * }
  */
 export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
-  /**
-   * A reference to a metrics aggregator
-   *
-   * @experimental Note this is alpha API. It may experience breaking changes in the future.
-   */
-  public metricsAggregator?: MetricsAggregator;
-
   /** Options passed to the SDK. */
   protected readonly _options: O;
 
@@ -280,9 +272,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
   public flush(timeout?: number): PromiseLike<boolean> {
     const transport = this._transport;
     if (transport) {
-      if (this.metricsAggregator) {
-        this.metricsAggregator.flush();
-      }
+      this.emit('flush');
       return this._isClientDoneProcessing(timeout).then(clientFinished => {
         return transport.flush(timeout).then(transportFlushed => clientFinished && transportFlushed);
       });
@@ -297,9 +287,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
   public close(timeout?: number): PromiseLike<boolean> {
     return this.flush(timeout).then(result => {
       this.getOptions().enabled = false;
-      if (this.metricsAggregator) {
-        this.metricsAggregator.close();
-      }
+      this.emit('close');
       return result;
     });
   }
@@ -495,6 +483,10 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
   /** @inheritdoc */
   public on(hook: 'startNavigationSpan', callback: (options: StartSpanOptions) => void): void;
 
+  public on(hook: 'flush', callback: () => void): void;
+
+  public on(hook: 'close', callback: () => void): void;
+
   /** @inheritdoc */
   public on(hook: string, callback: unknown): void {
     if (!this._hooks[hook]) {
@@ -541,6 +533,12 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
   /** @inheritdoc */
   public emit(hook: 'startNavigationSpan', options: StartSpanOptions): void;
 
+  /** @inheritdoc */
+  public emit(hook: 'flush'): void;
+
+  /** @inheritdoc */
+  public emit(hook: 'close'): void;
+
   /** @inheritdoc */
   public emit(hook: string, ...rest: unknown[]): void {
     if (this._hooks[hook]) {