Skip to content

Commit 36806f3

Browse files
timfishtimfish
authored
feat(metrics): Remove metrics method from BaseClient (#10789)
This PR removes the remaining metrics code from the `BaseClient`
1 parent dddac2c commit 36806f3

File tree

12 files changed

+79
-103
lines changed

12 files changed

+79
-103
lines changed

‎docs/event-sending.md

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ This document gives an outline for how event sending works, and which which plac
2525
- `createEnvelope()`
2626
- `addItemToEnvelope()`
2727
- `createAttachmentEnvelopeItem()`
28-
- `baseclient._sendEnvelope()`
28+
- `baseclient.sendEnvelope()`
2929
- `transport.send()`
3030

3131
## Transactions
@@ -54,7 +54,7 @@ This document gives an outline for how event sending works, and which which plac
5454
- `createEnvelope()`
5555
- `addItemToEnvelope()`
5656
- `createAttachmentEnvelopeItem()`
57-
- `baseclient._sendEnvelope()`
57+
- `baseclient.sendEnvelope()`
5858
- `transport.send()`
5959

6060
## Sessions
@@ -70,7 +70,7 @@ This document gives an outline for how event sending works, and which which plac
7070
- `createSessionEnvelope()`
7171
- `getSdkMetadataForEnvelopeHeader()`
7272
- `createEnvelope()`
73-
- `baseclient._sendEnvelope()`
73+
- `baseclient.sendEnvelope()`
7474
- `transport.send()`
7575
- `updateSession()`
7676

@@ -94,5 +94,5 @@ This document gives an outline for how event sending works, and which which plac
9494
- `browser.client._flushOutcomes()`
9595
- `getEnvelopeEndpointWithUrlEncodedAuth()`
9696
- `createClientReportEnvelope()`
97-
- `baseclient._sendEnvelope()`
97+
- `baseclient.sendEnvelope()`
9898
- `transport.send()`

‎packages/browser/src/client.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -96,9 +96,9 @@ export class BrowserClient extends BaseClient<BrowserClientOptions> {
9696
tunnel: this.getOptions().tunnel,
9797
});
9898

99-
// _sendEnvelope should not throw
99+
// sendEnvelope should not throw
100100
// eslint-disable-next-line @typescript-eslint/no-floating-promises
101-
this._sendEnvelope(envelope);
101+
this.sendEnvelope(envelope);
102102
}
103103

104104
/**
@@ -130,8 +130,8 @@ export class BrowserClient extends BaseClient<BrowserClientOptions> {
130130

131131
const envelope = createClientReportEnvelope(outcomes, this._options.tunnel && dsnToString(this._dsn));
132132

133-
// _sendEnvelope should not throw
133+
// sendEnvelope should not throw
134134
// eslint-disable-next-line @typescript-eslint/no-floating-promises
135-
this._sendEnvelope(envelope);
135+
this.sendEnvelope(envelope);
136136
}
137137
}

‎packages/core/src/baseclient.ts

Lines changed: 18 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@ import type {
1616
FeedbackEvent,
1717
Integration,
1818
IntegrationClass,
19-
MetricBucketItem,
2019
Outcome,
2120
ParameterizedString,
2221
SdkMetadata,
@@ -52,7 +51,6 @@ import { createEventEnvelope, createSessionEnvelope } from './envelope';
5251
import type { IntegrationIndex } from './integration';
5352
import { afterSetupIntegrations } from './integration';
5453
import { setupIntegration, setupIntegrations } from './integration';
55-
import { createMetricEnvelope } from './metrics/envelope';
5654
import type { Scope } from './scope';
5755
import { updateSession } from './session';
5856
import { getDynamicSamplingContextFromClient } from './tracing/dynamicSamplingContext';
@@ -377,7 +375,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
377375
env = addItemToEnvelope(env, createAttachmentEnvelopeItem(attachment));
378376
}
379377

380-
const promise = this._sendEnvelope(env);
378+
const promise = this.sendEnvelope(env);
381379
if (promise) {
382380
promise.then(sendResponse => this.emit('afterSendEvent', event, sendResponse), null);
383381
}
@@ -389,9 +387,9 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
389387
public sendSession(session: Session | SessionAggregates): void {
390388
const env = createSessionEnvelope(session, this._dsn, this._options._metadata, this._options.tunnel);
391389

392-
// _sendEnvelope should not throw
390+
// sendEnvelope should not throw
393391
// eslint-disable-next-line @typescript-eslint/no-floating-promises
394-
this._sendEnvelope(env);
392+
this.sendEnvelope(env);
395393
}
396394

397395
/**
@@ -415,23 +413,6 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
415413
}
416414
}
417415

418-
/**
419-
* @inheritDoc
420-
*/
421-
public captureAggregateMetrics(metricBucketItems: Array<MetricBucketItem>): void {
422-
DEBUG_BUILD && logger.log(`Flushing aggregated metrics, number of metrics: ${metricBucketItems.length}`);
423-
const metricsEnvelope = createMetricEnvelope(
424-
metricBucketItems,
425-
this._dsn,
426-
this._options._metadata,
427-
this._options.tunnel,
428-
);
429-
430-
// _sendEnvelope should not throw
431-
// eslint-disable-next-line @typescript-eslint/no-floating-promises
432-
this._sendEnvelope(metricsEnvelope);
433-
}
434-
435416
// Keep on() & emit() signatures in sync with types' client.ts interface
436417
/* eslint-disable @typescript-eslint/unified-signatures */
437418

@@ -540,6 +521,21 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
540521
}
541522
}
542523

524+
/**
525+
* @inheritdoc
526+
*/
527+
public sendEnvelope(envelope: Envelope): PromiseLike<void | TransportMakeRequestResponse> | void {
528+
this.emit('beforeEnvelope', envelope);
529+
530+
if (this._isEnabled() && this._transport) {
531+
return this._transport.send(envelope).then(null, reason => {
532+
DEBUG_BUILD && logger.error('Error while sending event:', reason);
533+
});
534+
} else {
535+
DEBUG_BUILD && logger.error('Transport disabled');
536+
}
537+
}
538+
543539
/* eslint-enable @typescript-eslint/unified-signatures */
544540

545541
/** Setup integrations for this client. */
@@ -823,21 +819,6 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
823819
);
824820
}
825821

826-
/**
827-
* @inheritdoc
828-
*/
829-
protected _sendEnvelope(envelope: Envelope): PromiseLike<void | TransportMakeRequestResponse> | void {
830-
this.emit('beforeEnvelope', envelope);
831-
832-
if (this._isEnabled() && this._transport) {
833-
return this._transport.send(envelope).then(null, reason => {
834-
DEBUG_BUILD && logger.error('Error while sending event:', reason);
835-
});
836-
} else {
837-
DEBUG_BUILD && logger.error('Transport disabled');
838-
}
839-
}
840-
841822
/**
842823
* Clears outcomes on this client and returns them.
843824
*/

‎packages/core/src/metrics/aggregator.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
11
import type {
2-
Client,
32
ClientOptions,
43
MeasurementUnit,
54
MetricsAggregator as MetricsAggregatorBase,
65
Primitive,
76
} from '@sentry/types';
87
import { timestampInSeconds } from '@sentry/utils';
8+
import type { BaseClient } from '../baseclient';
99
import { DEFAULT_FLUSH_INTERVAL, MAX_WEIGHT, NAME_AND_TAG_KEY_NORMALIZATION_REGEX, SET_METRIC_TYPE } from './constants';
10+
import { captureAggregateMetrics } from './envelope';
1011
import { METRIC_MAP } from './instance';
1112
import { updateMetricSummaryOnActiveSpan } from './metric-summary';
1213
import type { MetricBucket, MetricType } from './types';
@@ -39,7 +40,7 @@ export class MetricsAggregator implements MetricsAggregatorBase {
3940
// Force flush is used on either shutdown, flush() or when we exceed the max weight.
4041
private _forceFlush: boolean;
4142

42-
public constructor(private readonly _client: Client<ClientOptions>) {
43+
public constructor(private readonly _client: BaseClient<ClientOptions>) {
4344
this._buckets = new Map();
4445
this._bucketsTotalWeight = 0;
4546
this._interval = setInterval(() => this._flush(), DEFAULT_FLUSH_INTERVAL);
@@ -166,7 +167,7 @@ export class MetricsAggregator implements MetricsAggregatorBase {
166167
// TODO(@anonrig): Optimization opportunity.
167168
// This copy operation can be avoided if we store the key in the bucketItem.
168169
const buckets = Array.from(flushedBuckets).map(([, bucketItem]) => bucketItem);
169-
this._client.captureAggregateMetrics(buckets);
170+
captureAggregateMetrics(this._client, buckets);
170171
}
171172
}
172173
}

‎packages/core/src/metrics/browser-aggregator.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
1-
import type { Client, ClientOptions, MeasurementUnit, MetricsAggregator, Primitive } from '@sentry/types';
1+
import type { ClientOptions, MeasurementUnit, MetricsAggregator, Primitive } from '@sentry/types';
22
import { timestampInSeconds } from '@sentry/utils';
3+
import type { BaseClient } from '../baseclient';
34
import { DEFAULT_BROWSER_FLUSH_INTERVAL, NAME_AND_TAG_KEY_NORMALIZATION_REGEX, SET_METRIC_TYPE } from './constants';
5+
import { captureAggregateMetrics } from './envelope';
46
import { METRIC_MAP } from './instance';
57
import { updateMetricSummaryOnActiveSpan } from './metric-summary';
68
import type { MetricBucket, MetricType } from './types';
@@ -19,7 +21,7 @@ export class BrowserMetricsAggregator implements MetricsAggregator {
1921
private _buckets: MetricBucket;
2022
private readonly _interval: ReturnType<typeof setInterval>;
2123

22-
public constructor(private readonly _client: Client<ClientOptions>) {
24+
public constructor(private readonly _client: BaseClient<ClientOptions>) {
2325
this._buckets = new Map();
2426
this._interval = setInterval(() => this.flush(), DEFAULT_BROWSER_FLUSH_INTERVAL);
2527
}
@@ -80,7 +82,7 @@ export class BrowserMetricsAggregator implements MetricsAggregator {
8082

8183
// TODO(@anonrig): Use Object.values() when we support ES6+
8284
const metricBuckets = Array.from(this._buckets).map(([, bucketItem]) => bucketItem);
83-
this._client.captureAggregateMetrics(metricBuckets);
85+
captureAggregateMetrics(this._client, metricBuckets);
8486

8587
this._buckets.clear();
8688
}

‎packages/core/src/metrics/envelope.ts

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,34 @@
1-
import type { DsnComponents, MetricBucketItem, SdkMetadata, StatsdEnvelope, StatsdItem } from '@sentry/types';
2-
import { createEnvelope, dsnToString } from '@sentry/utils';
1+
import type {
2+
ClientOptions,
3+
DsnComponents,
4+
MetricBucketItem,
5+
SdkMetadata,
6+
StatsdEnvelope,
7+
StatsdItem,
8+
} from '@sentry/types';
9+
import { createEnvelope, dsnToString, logger } from '@sentry/utils';
10+
import type { BaseClient } from '../baseclient';
311
import { serializeMetricBuckets } from './utils';
412

13+
/**
14+
* Captures aggregated metrics to the supplied client.
15+
*/
16+
export function captureAggregateMetrics(
17+
client: BaseClient<ClientOptions>,
18+
metricBucketItems: Array<MetricBucketItem>,
19+
): void {
20+
logger.log(`Flushing aggregated metrics, number of metrics: ${metricBucketItems.length}`);
21+
const dsn = client.getDsn();
22+
const metadata = client.getSdkMetadata();
23+
const tunnel = client.getOptions().tunnel;
24+
25+
const metricsEnvelope = createMetricEnvelope(metricBucketItems, dsn, metadata, tunnel);
26+
27+
// sendEnvelope should not throw
28+
// eslint-disable-next-line @typescript-eslint/no-floating-promises
29+
client.sendEnvelope(metricsEnvelope);
30+
}
31+
532
/**
633
* Create envelope from a metric aggregate.
734
*/

‎packages/core/src/server-runtime-client.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -199,9 +199,9 @@ export class ServerRuntimeClient<
199199

200200
DEBUG_BUILD && logger.info('Sending checkin:', checkIn.monitorSlug, checkIn.status);
201201

202-
// _sendEnvelope should not throw
202+
// sendEnvelope should not throw
203203
// eslint-disable-next-line @typescript-eslint/no-floating-promises
204-
this._sendEnvelope(envelope);
204+
this.sendEnvelope(envelope);
205205

206206
return id;
207207
}

‎packages/core/test/lib/metrics/aggregator.test.ts

Lines changed: 5 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -81,45 +81,26 @@ describe('MetricsAggregator', () => {
8181

8282
describe('close', () => {
8383
test('should flush immediately', () => {
84-
const capture = jest.spyOn(testClient, 'captureAggregateMetrics');
84+
const capture = jest.spyOn(testClient, 'sendEnvelope');
8585
const aggregator = new MetricsAggregator(testClient);
8686
aggregator.add('c', 'requests', 1);
8787
aggregator.close();
8888
// It should clear the interval.
8989
expect(clearInterval).toHaveBeenCalled();
9090
expect(capture).toBeCalled();
9191
expect(capture).toBeCalledTimes(1);
92-
expect(capture).toBeCalledWith([
93-
{
94-
metric: { _value: 1 },
95-
metricType: 'c',
96-
name: 'requests',
97-
tags: {},
98-
timestamp: expect.any(Number),
99-
unit: 'none',
100-
},
101-
]);
10292
});
10393
});
10494

10595
describe('flush', () => {
10696
test('should flush immediately', () => {
107-
const capture = jest.spyOn(testClient, 'captureAggregateMetrics');
97+
const capture = jest.spyOn(testClient, 'sendEnvelope');
10898
const aggregator = new MetricsAggregator(testClient);
10999
aggregator.add('c', 'requests', 1);
110100
aggregator.flush();
111101
expect(capture).toBeCalled();
112102
expect(capture).toBeCalledTimes(1);
113-
expect(capture).toBeCalledWith([
114-
{
115-
metric: { _value: 1 },
116-
metricType: 'c',
117-
name: 'requests',
118-
tags: {},
119-
timestamp: expect.any(Number),
120-
unit: 'none',
121-
},
122-
]);
103+
123104
capture.mockReset();
124105
aggregator.close();
125106
// It should clear the interval.
@@ -130,7 +111,7 @@ describe('MetricsAggregator', () => {
130111
});
131112

132113
test('should not capture if empty', () => {
133-
const capture = jest.spyOn(testClient, 'captureAggregateMetrics');
114+
const capture = jest.spyOn(testClient, 'sendEnvelope');
134115
const aggregator = new MetricsAggregator(testClient);
135116
aggregator.add('c', 'requests', 1);
136117
aggregator.flush();
@@ -143,7 +124,7 @@ describe('MetricsAggregator', () => {
143124

144125
describe('add', () => {
145126
test('it should respect the max weight and flush if exceeded', () => {
146-
const capture = jest.spyOn(testClient, 'captureAggregateMetrics');
127+
const capture = jest.spyOn(testClient, 'sendEnvelope');
147128
const aggregator = new MetricsAggregator(testClient);
148129

149130
for (let i = 0; i < MAX_WEIGHT; i++) {

‎packages/core/test/lib/serverruntimeclient.test.ts

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -78,8 +78,7 @@ describe('ServerRuntimeClient', () => {
7878
});
7979
client = new ServerRuntimeClient(options);
8080

81-
// @ts-expect-error accessing private method
82-
const sendEnvelopeSpy = jest.spyOn(client, '_sendEnvelope');
81+
const sendEnvelopeSpy = jest.spyOn(client, 'sendEnvelope');
8382

8483
const id = client.captureCheckIn(
8584
{ monitorSlug: 'foo', status: 'in_progress' },
@@ -145,8 +144,7 @@ describe('ServerRuntimeClient', () => {
145144
const options = getDefaultClientOptions({ dsn: PUBLIC_DSN, serverName: 'bar', enabled: false });
146145
client = new ServerRuntimeClient(options);
147146

148-
// @ts-expect-error accessing private method
149-
const sendEnvelopeSpy = jest.spyOn(client, '_sendEnvelope');
147+
const sendEnvelopeSpy = jest.spyOn(client, 'sendEnvelope');
150148

151149
client.captureCheckIn({ monitorSlug: 'foo', status: 'in_progress' });
152150

‎packages/node-experimental/test/sdk/client.test.ts

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -393,8 +393,7 @@ describe('NodeClient', () => {
393393
});
394394
const client = new NodeClient(options);
395395

396-
// @ts-expect-error accessing private method
397-
const sendEnvelopeSpy = jest.spyOn(client, '_sendEnvelope');
396+
const sendEnvelopeSpy = jest.spyOn(client, 'sendEnvelope');
398397

399398
const id = client.captureCheckIn(
400399
{ monitorSlug: 'foo', status: 'in_progress' },
@@ -464,8 +463,7 @@ describe('NodeClient', () => {
464463
});
465464
const client = new NodeClient(options);
466465

467-
// @ts-expect-error accessing private method
468-
const sendEnvelopeSpy = jest.spyOn(client, '_sendEnvelope');
466+
const sendEnvelopeSpy = jest.spyOn(client, 'sendEnvelope');
469467

470468
const id = client.captureCheckIn({ monitorSlug: 'heartbeat-monitor', status: 'ok' });
471469

@@ -491,8 +489,7 @@ describe('NodeClient', () => {
491489
const options = getDefaultNodeClientOptions({ serverName: 'bar', enabled: false });
492490
const client = new NodeClient(options);
493491

494-
// @ts-expect-error accessing private method
495-
const sendEnvelopeSpy = jest.spyOn(client, '_sendEnvelope');
492+
const sendEnvelopeSpy = jest.spyOn(client, 'sendEnvelope');
496493

497494
client.captureCheckIn({ monitorSlug: 'foo', status: 'in_progress' });
498495

0 commit comments

Comments
 (0)