8000 feat(core): Decouple metrics aggregation from client (#10628) · GingerAdonis/sentry-javascript@a8966d1 · GitHub
[go: up one dir, main page]
Skip to content

Commit a8966d1

Browse files
10000 timfishtimfish
authored
feat(core): Decouple metrics aggregation from client (getsentry#10628)
This is a re-opening of getsentry#10596 which was closed automatically after the branch is was based off was closed. Part of the work for getsentry#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`.
1 parent 2334f24 commit a8966d1

File tree

21 files changed

+238
-80
lines changed

21 files changed

+238
-80
lines changed

‎MIGRATION.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
# Upgrading from 7.x to 8.x
22

3+
## Removal of the `MetricsAggregator` integration class and `metricsAggregatorIntegration`
4+
5+
The SDKs now support metrics features without any additional configuration.
6+
37
## Updated behaviour of `tracePropagationTargets` in the browser (HTTP tracing headers & CORS)
48

59
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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
import * as Sentry from '@sentry/browser';
2+
3+
window.Sentry = Sentry;
4+
5+
Sentry.init({
6+
dsn: 'https://public@dsn.ingest.sentry.io/1337',
7+
});
8+
9+
Sentry.metrics.increment('increment');
10+
Sentry.metrics.increment('increment');
11+
Sentry.metrics.distribution('distribution', 42);
12+
Sentry.metrics.distribution('distribution', 45);
13+
Sentry.metrics.gauge('gauge', 5);
14+
Sentry.metrics.gauge('gauge', 15);
15+
Sentry.metrics.set('set', 'nope');
16+
Sentry.metrics.set('set', 'another');

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
import { expect } from '@playwright/test';
2+
3+
import { sentryTest } from '../../utils/fixtures';
4+
import { getFirstSentryEnvelopeRequest, properEnvelopeRequestParser } from '../../utils/helpers';
5+
6+
sentryTest('collects metrics', async ({ getLocalTestUrl, page }) => {
7+
const url = await getLocalTestUrl({ testDir: __dirname });
8+
9+
const statsdBuffer = await getFirstSentryEnvelopeRequest<Uint8Array>(page, url, properEnvelopeRequestParser);
10+
const statsdString = new TextDecoder().decode(statsdBuffer);
11+
// Replace all the Txxxxxx to remove the timestamps
12+
const normalisedStatsdString = statsdString.replace(/T\d+\n?/g, 'T000000');
13+
14+
expect(normalisedStatsdString).toEqual(
15+
'increment@none:2|c|T000000distribution@none:42:45|d|T000000gauge@none:15:5:15:20:2|g|T000000set@none:3387254:3443787523|s|T000000',
16+
);
17+
});

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

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import type { Page, Request } from '@playwright/test';
2-
import type { EnvelopeItemType, Event, EventEnvelopeHeaders } from '@sentry/types';
2+
import type { EnvelopeItem, EnvelopeItemType, Event, EventEnvelopeHeaders } from '@sentry/types';
3+
import { parseEnvelope } from '@sentry/utils';
34

45
export const envelopeUrlRegex = /\.sentry\.io\/api\/\d+\/envelope\//;
56

@@ -21,6 +22,27 @@ export const envelopeRequestParser = <T = Event>(request: Request | null, envelo
2122
return envelopeParser(request)[envelopeIndex] as T;
2223
};
2324

25+
/**
26+
* The above envelope parser does not follow the envelope spec...
27+
* ...but modifying it to follow the spec breaks a lot of the test which rely on the current indexing behavior.
28+
*
29+
* This parser is a temporary solution to allow us to test metrics with statsd envelopes.
30+
*
31+
* Eventually, all the tests should be migrated to use this 'proper' envelope parser!
32+
*/
33+
export const properEnvelopeParser = (request: Request | null): EnvelopeItem[] => {
34+
// https://develop.sentry.dev/sdk/envelopes/
35+
const envelope = request?.postData() || '';
36+
37+
const [, items] = parseEnvelope(envelope, new TextEncoder(), new TextDecoder());
38+
39+
return items;
40+
};
41+
42+
export const properEnvelopeRequestParser = <T = Event>(request: Request | null, envelopeIndex = 1): T => {
43+
return properEnvelopeParser(request)[0][envelopeIndex] as T;
44+
};
45+
2446
export const envelopeHeaderRequestParser = (request: Request | null): EventEnvelopeHeaders => {
2547
// https://develop.sentry.dev/sdk/envelopes/
2648
const envelope = request?.postData() || '';

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

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,6 @@ Sentry.init({
66
release: '1.0',
77
tracesSampleRate: 1.0,
88
transport: loggingTransport,
9-
_experiments: {
10-
metricsAggregator: true,
11-
},
129
});
1310

1411
// Stop the process from exiting before the transaction is sent

‎packages/astro/src/index.types.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,4 +26,5 @@ export declare const defaultStackParser: StackParser;
2626
export declare function close(timeout?: number | undefined): PromiseLike<boolean>;
2727
export declare function flush(timeout?: number | undefined): PromiseLike<boolean>;
2828

29+
export declare const metrics: typeof clientSdk.metrics & typeof serverSdk.metrics;
2930
export default sentryAstro;

‎packages/browser/src/exports.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,6 @@ export {
6464
FunctionToString,
6565
// eslint-disable-next-line deprecation/deprecation
6666
InboundFilters,
67-
metrics,
6867
functionToStringIntegration,
6968
inboundFiltersIntegration,
7069
parameterize,
@@ -77,6 +76,8 @@ export {
7776
SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE,
7877
} from '@sentry/core';
7978

79+
export * from './metrics';
80+
8081
export { WINDOW } from './helpers';
8182
export { BrowserClient } from './client';
8283
export { makeFetchTransport, makeXHRTransport } from './transports';

‎packages/browser/src/metrics.ts

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
import type { MetricData } from '@sentry/core';
2+
import { BrowserMetricsAggregator, metrics as metricsCore } from '@sentry/core';
3+
4+
/**
5+
* Adds a value to a counter metric
6+
*
7+
* @experimental This API is experimental and might have breaking changes in the future.
8+
*/
9+
function increment(name: string, value: number = 1, data?: MetricData): void {
10+
metricsCore.increment(BrowserMetricsAggregator, name, value, data);
11+
}
12+
13+
/**
14+
* Adds a value to a distribution metric
15+
*
16+
* @experimental This API is experimental and might have breaking changes in the future.
17+
*/
18+
function distribution(name: string, value: number, data?: MetricData): void {
19+
metricsCore.distribution(BrowserMetricsAggregator, name, value, data);
20+
}
21+
22+
/**
23+
* Adds a value to a set metric. Value must be a string or integer.
24+
*
25+
* @experimental This API is experimental and might have breaking changes in the future.
26+
*/
27+
function set(name: string, value: number | string, data?: MetricData): void {
28+
metricsCore.set(BrowserMetricsAggregator, name, value, data);
29+
}
30+
31+
/**
32+
* Adds a value to a gauge metric
33+
*
34+
* @experimental This API is experimental and might have breaking changes in the future.
35+
*/
36+
function gauge(name: string, value: number, data?: MetricData): void {
37+
metricsCore.gauge(BrowserMetricsAggregator, name, value, data);
38+
}
39+
40+
export const metrics = {
41+
increment,
42+
distribution,
43+
set,
44+
gauge,
45+
};

‎packages/bun/src/index.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ export {
7171
startInactiveSpan,
7272
startSpanManual,
7373
continueTrace,
74-
metrics,
74+
metricsDefault as metrics,
7575
functionToStringIntegration,
7676
inboundFiltersIntegration,
7777
linkedErrorsIntegration,

‎packages/core/src/baseclient.ts

Lines changed: 12 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ import type {
1717
Integration,
1818
IntegrationClass,
1919
MetricBucketItem,
20-
MetricsAggregator,
2120
Outcome,
2221
ParameterizedString,
2322
SdkMetadata,
@@ -93,13 +92,6 @@ const ALREADY_SEEN_ERROR = "Not capturing exception because it's already been ca
9392
* }
9493
*/
9594
export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
96-
/**
97-
* A reference to a metrics aggregator
98-
*
99-
* @experimental Note this is alpha API. It may experience breaking changes in the future.
100-
*/
101-
public metricsAggregator?: MetricsAggregator;
102-
10395
/** Options passed to the SDK. */
10496
protected readonly _options: O;
10597

@@ -280,9 +272,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
280272
public flush(timeout?: number): PromiseLike<boolean> {
281273
const transport = this._transport;
282274
if (transport) {
283-
if (this.metricsAggregator) {
284-
this.metricsAggregator.flush();
285-
}
275+
this.emit('flush');
286276
return this._isClientDoneProcessing(timeout).then(clientFinished => {
287277
return transport.flush(timeout).then(transportFlushed => clientFinished && transportFlushed);
288278
});
@@ -297,9 +287,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
297287
public close(timeout?: number): PromiseLike<boolean> {
298288
return this.flush(timeout).then(result => {
299289
this.getOptions().enabled = false;
300-
if (this.metricsAggregator) {
301-
this.metricsAggregator.close();
302-
}
290+
this.emit('close');
303291
return result;
304292
});
305293
}
@@ -495,6 +483,10 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
495483
/** @inheritdoc */
496484
public on(hook: 'startNavigationSpan', callback: (options: StartSpanOptions) => void): void;
497485

486+
public on(hook: 'flush', callback: () => void): void;
487+
488+
public on(hook: 'close', callback: () => void): void;
489+
498490
/** @inheritdoc */
499491
public on(hook: string, callback: unknown): void {
500492
if (!this._hooks[hook]) {
@@ -541,6 +533,12 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
541533
/** @inheritdoc */
542534
public emit(hook: 'startNavigationSpan', options: StartSpanOptions): void;
543535

536+
/** @inheritdoc */
537+
public emit(hook: 'flush'): void;
538+
539+
/** @inheritdoc */
540+
public emit(hook: 'close'): void;
541+
544542
/** @inheritdoc */
545543
public emit(hook: string, ...rest: unknown[]): void {
546544
if (this._hooks[hook]) {

‎packages/core/src/index.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,9 @@ export { linkedErrorsIntegration } from './integrations/linkederrors';
114114
export { moduleMetadataIntegration } from './integrations/metadata';
115115
export { requestDataIntegration } from './integrations/requestdata';
116116
export { metrics } from './metrics/exports';
117+
export type { MetricData } from './metrics/exports';
118+
export { metricsDefault } from './metrics/exports-default';
119+
export { BrowserMetricsAggregator } from './metrics/browser-aggregator';
117120

118121
/** @deprecated Import the integration function directly, e.g. `inboundFiltersIntegration()` instead of `new Integrations.InboundFilter(). */
119122
const Integrations = INTEGRATIONS;

‎packages/core/src/metrics/exports-default.ts

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
import { MetricsAggregator } from './aggregator';
2+
import type { MetricData } from './exports';
3+
import { metrics as metricsCore } from './exports';
4+
5+
/**
6+
* Adds a value to a counter metric
7+
*
8+
* @experimental This API is experimental and might have breaking changes in the future.
9+
*/
10+
function increment(name: string, value: number = 1, data?: MetricData): void {
11+
metricsCore.increment(MetricsAggregator, name, value, data);
12+
}
13+
14+
/**
15+
* Adds a value to a distribution metric
16+
*
17+
* @experimental This API is experimental and might have breaking changes in the future.
18+
*/
19+
function distribution(name: string, value: number, data?: MetricData): void {
20+
metricsCore.distribution(MetricsAggregator, name, value, data);
21+
}
22+
23+
/**
24+
* Adds a value to a set metric. Value must be a string or integer.
25+
*
26+
* @experimental This API is experimental and might have breaking changes in the future.
27+
*/
28+
function set(name: string, value: number | string, data?: MetricData): void {
29+
metricsCore.set(MetricsAggregator, name, value, data);
30+
}
31+
32+
/**
33+
* Adds a value to a gauge metric
34+
*
35+
* @experimental This API is experimental and might have breaking changes in the future.
36+
*/
37+
function gauge(name: string, value: number, data?: MetricData): void {
38+
metricsCore.gauge(MetricsAggregator, name, value, data);
39+
}
40+
41+
export const metricsDefault = {
42+
increment,
43+
distribution,
44+
set,
45+
gauge,
46+
};

0 commit comments

Comments
 (0)
0