feat(metrics): Remove metrics method from BaseClient (#10789)

This PR removes the remaining metrics code from the `BaseClient`
getsentry · Feb 22, 2024 · 36806f3 · 36806f3
1 parent dddac2c
commit 36806f3
Show file tree

Hide file tree

Showing 12 changed files with 79 additions and 103 deletions.
diff --git a/docs/event-sending.md b/docs/event-sending.md
@@ -25,7 +25,7 @@ This document gives an outline for how event sending works, and which which plac
             - `createEnvelope()`
           - `addItemToEnvelope()`
             - `createAttachmentEnvelopeItem()`
-          - `baseclient._sendEnvelope()`
+          - `baseclient.sendEnvelope()`
             - `transport.send()`
 
 ## Transactions
@@ -54,7 +54,7 @@ This document gives an outline for how event sending works, and which which plac
             - `createEnvelope()`
           - `addItemToEnvelope()`
             - `createAttachmentEnvelopeItem()`
-          - `baseclient._sendEnvelope()`
+          - `baseclient.sendEnvelope()`
             - `transport.send()`
 
 ## Sessions
@@ -70,7 +70,7 @@ This document gives an outline for how event sending works, and which which plac
         - `createSessionEnvelope()`
           - `getSdkMetadataForEnvelopeHeader()`
           - `createEnvelope()`
-        - `baseclient._sendEnvelope()`
+        - `baseclient.sendEnvelope()`
           - `transport.send()`
       - `updateSession()`
 
@@ -94,5 +94,5 @@ This document gives an outline for how event sending works, and which which plac
   - `browser.client._flushOutcomes()`
     - `getEnvelopeEndpointWithUrlEncodedAuth()`
     - `createClientReportEnvelope()`
-    - `baseclient._sendEnvelope()`
+    - `baseclient.sendEnvelope()`
       - `transport.send()`
diff --git a/packages/browser/src/client.ts b/packages/browser/src/client.ts
@@ -96,9 +96,9 @@ export class BrowserClient extends BaseClient<BrowserClientOptions> {
       tunnel: this.getOptions().tunnel,
     });
 
-    // _sendEnvelope should not throw
+    // sendEnvelope should not throw
     // eslint-disable-next-line @typescript-eslint/no-floating-promises
-    this._sendEnvelope(envelope);
+    this.sendEnvelope(envelope);
   }
 
   /**
@@ -130,8 +130,8 @@ export class BrowserClient extends BaseClient<BrowserClientOptions> {
 
     const envelope = createClientReportEnvelope(outcomes, this._options.tunnel && dsnToString(this._dsn));
 
-    // _sendEnvelope should not throw
+    // sendEnvelope should not throw
     // eslint-disable-next-line @typescript-eslint/no-floating-promises
-    this._sendEnvelope(envelope);
+    this.sendEnvelope(envelope);
   }
 }
diff --git a/packages/core/src/baseclient.ts b/packages/core/src/baseclient.ts
@@ -16,7 +16,6 @@ import type {
   FeedbackEvent,
   Integration,
   IntegrationClass,
-  MetricBucketItem,
   Outcome,
   ParameterizedString,
   SdkMetadata,
@@ -52,7 +51,6 @@ import { createEventEnvelope, createSessionEnvelope } from './envelope';
 import type { IntegrationIndex } from './integration';
 import { afterSetupIntegrations } from './integration';
 import { setupIntegration, setupIntegrations } from './integration';
-import { createMetricEnvelope } from './metrics/envelope';
 import type { Scope } from './scope';
 import { updateSession } from './session';
 import { getDynamicSamplingContextFromClient } from './tracing/dynamicSamplingContext';
@@ -377,7 +375,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
       env = addItemToEnvelope(env, createAttachmentEnvelopeItem(attachment));
     }
 
-    const promise = this._sendEnvelope(env);
+    const promise = this.sendEnvelope(env);
     if (promise) {
       promise.then(sendResponse => this.emit('afterSendEvent', event, sendResponse), null);
     }
@@ -389,9 +387,9 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
   public sendSession(session: Session | SessionAggregates): void {
     const env = createSessionEnvelope(session, this._dsn, this._options._metadata, this._options.tunnel);
 
-    // _sendEnvelope should not throw
+    // sendEnvelope should not throw
     // eslint-disable-next-line @typescript-eslint/no-floating-promises
-    this._sendEnvelope(env);
+    this.sendEnvelope(env);
   }
 
   /**
@@ -415,23 +413,6 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
     }
   }
 
-  /**
-   * @inheritDoc
-   */
-  public captureAggregateMetrics(metricBucketItems: Array<MetricBucketItem>): void {
-    DEBUG_BUILD && logger.log(`Flushing aggregated metrics, number of metrics: ${metricBucketItems.length}`);
-    const metricsEnvelope = createMetricEnvelope(
-      metricBucketItems,
-      this._dsn,
-      this._options._metadata,
-      this._options.tunnel,
-    );
-
-    // _sendEnvelope should not throw
-    // eslint-disable-next-line @typescript-eslint/no-floating-promises
-    this._sendEnvelope(metricsEnvelope);
-  }
-
   // Keep on() & emit() signatures in sync with types' client.ts interface
   /* eslint-disable @typescript-eslint/unified-signatures */
 
@@ -540,6 +521,21 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
     }
   }
 
+  /**
+   * @inheritdoc
+   */
+  public sendEnvelope(envelope: Envelope): PromiseLike<void | TransportMakeRequestResponse> | void {
+    this.emit('beforeEnvelope', envelope);
+
+    if (this._isEnabled() && this._transport) {
+      return this._transport.send(envelope).then(null, reason => {
+        DEBUG_BUILD && logger.error('Error while sending event:', reason);
+      });
+    } else {
+      DEBUG_BUILD && logger.error('Transport disabled');
+    }
+  }
+
   /* eslint-enable @typescript-eslint/unified-signatures */
 
   /** Setup integrations for this client. */
@@ -823,21 +819,6 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
     );
   }
 
-  /**
-   * @inheritdoc
-   */
-  protected _sendEnvelope(envelope: Envelope): PromiseLike<void | TransportMakeRequestResponse> | void {
-    this.emit('beforeEnvelope', envelope);
-
-    if (this._isEnabled() && this._transport) {
-      return this._transport.send(envelope).then(null, reason => {
-        DEBUG_BUILD && logger.error('Error while sending event:', reason);
-      });
-    } else {
-      DEBUG_BUILD && logger.error('Transport disabled');
-    }
-  }
-
   /**
    * Clears outcomes on this client and returns them.
    */

diff --git a/packages/core/src/metrics/aggregator.ts b/packages/core/src/metrics/aggregator.ts
@@ -1,12 +1,13 @@
 import type {
-  Client,
   ClientOptions,
   MeasurementUnit,
   MetricsAggregator as MetricsAggregatorBase,
   Primitive,
 } from '@sentry/types';
 import { timestampInSeconds } from '@sentry/utils';
+import type { BaseClient } from '../baseclient';
 import { DEFAULT_FLUSH_INTERVAL, MAX_WEIGHT, NAME_AND_TAG_KEY_NORMALIZATION_REGEX, SET_METRIC_TYPE } from './constants';
+import { captureAggregateMetrics } from './envelope';
 import { METRIC_MAP } from './instance';
 import { updateMetricSummaryOnActiveSpan } from './metric-summary';
 import type { MetricBucket, MetricType } from './types';
@@ -39,7 +40,7 @@ export class MetricsAggregator implements MetricsAggregatorBase {
   // Force flush is used on either shutdown, flush() or when we exceed the max weight.
   private _forceFlush: boolean;
 
-  public constructor(private readonly _client: Client<ClientOptions>) {
+  public constructor(private readonly _client: BaseClient<ClientOptions>) {
     this._buckets = new Map();
     this._bucketsTotalWeight = 0;
     this._interval = setInterval(() => this._flush(), DEFAULT_FLUSH_INTERVAL);
@@ -166,7 +167,7 @@ export class MetricsAggregator implements MetricsAggregatorBase {
       // TODO(@anonrig): Optimization opportunity.
       // This copy operation can be avoided if we store the key in the bucketItem.
       const buckets = Array.from(flushedBuckets).map(([, bucketItem]) => bucketItem);
-      this._client.captureAggregateMetrics(buckets);
+      captureAggregateMetrics(this._client, buckets);
     }
   }
 }
diff --git a/packages/core/src/metrics/browser-aggregator.ts b/packages/core/src/metrics/browser-aggregator.ts
@@ -1,6 +1,8 @@
-import type { Client, ClientOptions, MeasurementUnit, MetricsAggregator, Primitive } from '@sentry/types';
+import type { ClientOptions, MeasurementUnit, MetricsAggregator, Primitive } from '@sentry/types';
 import { timestampInSeconds } from '@sentry/utils';
+import type { BaseClient } from '../baseclient';
 import { DEFAULT_BROWSER_FLUSH_INTERVAL, NAME_AND_TAG_KEY_NORMALIZATION_REGEX, SET_METRIC_TYPE } from './constants';
+import { captureAggregateMetrics } from './envelope';
 import { METRIC_MAP } from './instance';
 import { updateMetricSummaryOnActiveSpan } from './metric-summary';
 import type { MetricBucket, MetricType } from './types';
@@ -19,7 +21,7 @@ export class BrowserMetricsAggregator implements MetricsAggregator {
   private _buckets: MetricBucket;
   private readonly _interval: ReturnType<typeof setInterval>;
 
-  public constructor(private readonly _client: Client<ClientOptions>) {
+  public constructor(private readonly _client: BaseClient<ClientOptions>) {
     this._buckets = new Map();
     this._interval = setInterval(() => this.flush(), DEFAULT_BROWSER_FLUSH_INTERVAL);
   }
@@ -80,7 +82,7 @@ export class BrowserMetricsAggregator implements MetricsAggregator {
 
     // TODO(@anonrig): Use Object.values() when we support ES6+
     const metricBuckets = Array.from(this._buckets).map(([, bucketItem]) => bucketItem);
-    this._client.captureAggregateMetrics(metricBuckets);
+    captureAggregateMetrics(this._client, metricBuckets);
 
     this._buckets.clear();
   }

diff --git a/packages/core/src/metrics/envelope.ts b/packages/core/src/metrics/envelope.ts
@@ -1,7 +1,34 @@
-import type { DsnComponents, MetricBucketItem, SdkMetadata, StatsdEnvelope, StatsdItem } from '@sentry/types';
-import { createEnvelope, dsnToString } from '@sentry/utils';
+import type {
+  ClientOptions,
+  DsnComponents,
+  MetricBucketItem,
+  SdkMetadata,
+  StatsdEnvelope,
+  StatsdItem,
+} from '@sentry/types';
+import { createEnvelope, dsnToString, logger } from '@sentry/utils';
+import type { BaseClient } from '../baseclient';
 import { serializeMetricBuckets } from './utils';
 
+/**
+ * Captures aggregated metrics to the supplied client.
+ */
+export function captureAggregateMetrics(
+  client: BaseClient<ClientOptions>,
+  metricBucketItems: Array<MetricBucketItem>,
+): void {
+  logger.log(`Flushing aggregated metrics, number of metrics: ${metricBucketItems.length}`);
+  const dsn = client.getDsn();
+  const metadata = client.getSdkMetadata();
+  const tunnel = client.getOptions().tunnel;
+
+  const metricsEnvelope = createMetricEnvelope(metricBucketItems, dsn, metadata, tunnel);
+
+  // sendEnvelope should not throw
+  // eslint-disable-next-line @typescript-eslint/no-floating-promises
+  client.sendEnvelope(metricsEnvelope);
+}
+
 /**
  * Create envelope from a metric aggregate.
  */

diff --git a/packages/core/src/server-runtime-client.ts b/packages/core/src/server-runtime-client.ts
@@ -199,9 +199,9 @@ export class ServerRuntimeClient<
 
     DEBUG_BUILD && logger.info('Sending checkin:', checkIn.monitorSlug, checkIn.status);
 
-    // _sendEnvelope should not throw
+    // sendEnvelope should not throw
     // eslint-disable-next-line @typescript-eslint/no-floating-promises
-    this._sendEnvelope(envelope);
+    this.sendEnvelope(envelope);
 
     return id;
   }

diff --git a/packages/core/test/lib/metrics/aggregator.test.ts b/packages/core/test/lib/metrics/aggregator.test.ts
@@ -81,45 +81,26 @@ describe('MetricsAggregator', () => {
 
   describe('close', () => {
     test('should flush immediately', () => {
-      const capture = jest.spyOn(testClient, 'captureAggregateMetrics');
+      const capture = jest.spyOn(testClient, 'sendEnvelope');
       const aggregator = new MetricsAggregator(testClient);
       aggregator.add('c', 'requests', 1);
       aggregator.close();
       // It should clear the interval.
       expect(clearInterval).toHaveBeenCalled();
       expect(capture).toBeCalled();
       expect(capture).toBeCalledTimes(1);
-      expect(capture).toBeCalledWith([
-        {
-          metric: { _value: 1 },
-          metricType: 'c',
-          name: 'requests',
-          tags: {},
-          timestamp: expect.any(Number),
-          unit: 'none',
-        },
-      ]);
     });
   });
 
   describe('flush', () => {
     test('should flush immediately', () => {
-      const capture = jest.spyOn(testClient, 'captureAggregateMetrics');
+      const capture = jest.spyOn(testClient, 'sendEnvelope');
       const aggregator = new MetricsAggregator(testClient);
       aggregator.add('c', 'requests', 1);
       aggregator.flush();
       expect(capture).toBeCalled();
       expect(capture).toBeCalledTimes(1);
-      expect(capture).toBeCalledWith([
-        {
-          metric: { _value: 1 },
-          metricType: 'c',
-          name: 'requests',
-          tags: {},
-          timestamp: expect.any(Number),
-          unit: 'none',
-        },
-      ]);
+
       capture.mockReset();
       aggregator.close();
       // It should clear the interval.
@@ -130,7 +111,7 @@ describe('MetricsAggregator', () => {
     });
 
     test('should not capture if empty', () => {
-      const capture = jest.spyOn(testClient, 'captureAggregateMetrics');
+      const capture = jest.spyOn(testClient, 'sendEnvelope');
       const aggregator = new MetricsAggregator(testClient);
       aggregator.add('c', 'requests', 1);
       aggregator.flush();
@@ -143,7 +124,7 @@ describe('MetricsAggregator', () => {
 
   describe('add', () => {
     test('it should respect the max weight and flush if exceeded', () => {
-      const capture = jest.spyOn(testClient, 'captureAggregateMetrics');
+      const capture = jest.spyOn(testClient, 'sendEnvelope');
       const aggregator = new MetricsAggregator(testClient);
 
       for (let i = 0; i < MAX_WEIGHT; i++) {

diff --git a/packages/core/test/lib/serverruntimeclient.test.ts b/packages/core/test/lib/serverruntimeclient.test.ts
@@ -78,8 +78,7 @@ describe('ServerRuntimeClient', () => {
       });
       client = new ServerRuntimeClient(options);
 
-      // @ts-expect-error accessing private method
-      const sendEnvelopeSpy = jest.spyOn(client, '_sendEnvelope');
+      const sendEnvelopeSpy = jest.spyOn(client, 'sendEnvelope');
 
       const id = client.captureCheckIn(
         { monitorSlug: 'foo', status: 'in_progress' },
@@ -145,8 +144,7 @@ describe('ServerRuntimeClient', () => {
       const options = getDefaultClientOptions({ dsn: PUBLIC_DSN, serverName: 'bar', enabled: false });
       client = new ServerRuntimeClient(options);
 
-      // @ts-expect-error accessing private method
-      const sendEnvelopeSpy = jest.spyOn(client, '_sendEnvelope');
+      const sendEnvelopeSpy = jest.spyOn(client, 'sendEnvelope');
 
       client.captureCheckIn({ monitorSlug: 'foo', status: 'in_progress' });
 

diff --git a/packages/node-experimental/test/sdk/client.test.ts b/packages/node-experimental/test/sdk/client.test.ts
@@ -393,8 +393,7 @@ describe('NodeClient', () => {
       });
       const client = new NodeClient(options);
 
-      // @ts-expect-error accessing private method
-      const sendEnvelopeSpy = jest.spyOn(client, '_sendEnvelope');
+      const sendEnvelopeSpy = jest.spyOn(client, 'sendEnvelope');
 
       const id = client.captureCheckIn(
         { monitorSlug: 'foo', status: 'in_progress' },
@@ -464,8 +463,7 @@ describe('NodeClient', () => {
       });
       const client = new NodeClient(options);
 
-      // @ts-expect-error accessing private method
-      const sendEnvelopeSpy = jest.spyOn(client, '_sendEnvelope');
+      const sendEnvelopeSpy = jest.spyOn(client, 'sendEnvelope');
 
       const id = client.captureCheckIn({ monitorSlug: 'heartbeat-monitor', status: 'ok' });
 
@@ -491,8 +489,7 @@ describe('NodeClient', () => {
       const options = getDefaultNodeClientOptions({ serverName: 'bar', enabled: false });
       const client = new NodeClient(options);
 
-      // @ts-expect-error accessing private method
-      const sendEnvelopeSpy = jest.spyOn(client, '_sendEnvelope');
+      const sendEnvelopeSpy = jest.spyOn(client, 'sendEnvelope');
 
       client.captureCheckIn({ monitorSlug: 'foo', status: 'in_progress' });