Skip to content
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

feat(tracing): Implement new browserTracingIntegration() #10327

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions MIGRATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,33 @@ npx @sentry/migr8@latest
This will let you select which updates to run, and automatically update your code. Make sure to still review all code
changes!

## Deprecate `new BrowserTracing()` in favor of `browserTracingIntegration()`

In v8, you have to use the functional style for the browser tracing integration. This works mostly the same, but some of
the options have changed:

- `startTransactionOnPageLoad` --> `instrumentPageLoad`
- `startTransactionOnLocationChange` --> `instrumentNavigation`
- `markBackgroundTransactions` --> `markBackgroundSpan`
- `beforeNavigate` --> `beforeStartSpan`

Finally, instead of `routingInstrumentation`, you have to disable instrumentation via e.g.
`instrumentNavigation: false`, and can then manually emit events like this:

```js
// Example router event
router.on('routeChange', route => {
Sentry.getClient().emit('startNavigationSpan', {
name: route.name,
op: 'navigation',
});

const activeSpan = Sentry.getActiveSpan(); // <-- this will hold the navigation span
});
```

The new `browserTracingIntegration()` will pick these up and create the correct spans.

## Deprecate using `getClient()` to check if the SDK was initialized

In v8, `getClient()` will stop returning `undefined` if `Sentry.init()` was not called. For cases where this may be used
Expand Down
5 changes: 2 additions & 3 deletions packages/angular/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -93,14 +93,13 @@ Registering a Trace Service is a 3-step process.
instrumentation:

```javascript
import { init, instrumentAngularRouting, BrowserTracing } from '@sentry/angular';
import { init, browserTracingIntegration } from '@sentry/angular';

init({
dsn: '__DSN__',
integrations: [
new BrowserTracing({
browserTracingIntegration({
tracingOrigins: ['localhost', 'https://yourserver.io/api'],
routingInstrumentation: instrumentAngularRouting,
}),
],
tracesSampleRate: 1,
Expand Down
1 change: 1 addition & 0 deletions packages/angular/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export {
// TODO `instrumentAngularRouting` is just an alias for `routingInstrumentation`; deprecate the latter at some point
instrumentAngularRouting, // new name
routingInstrumentation, // legacy name
browserTracingIntegration,
TraceClassDecorator,
TraceMethodDecorator,
TraceDirective,
Expand Down
69 changes: 63 additions & 6 deletions packages/angular/src/tracing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,14 @@ import type { ActivatedRouteSnapshot, Event, RouterState } from '@angular/router
// eslint-disable-next-line @typescript-eslint/consistent-type-imports
import { NavigationCancel, NavigationError, Router } from '@angular/router';
import { NavigationEnd, NavigationStart, ResolveEnd } from '@angular/router';
import { WINDOW, getCurrentScope } from '@sentry/browser';
import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, spanToJSON } from '@sentry/core';
import type { Span, Transaction, TransactionContext } from '@sentry/types';
import {
WINDOW,
browserTracingIntegration as originalBrowserTracingIntegration,
browserTracingStartNavigationSpan,
getCurrentScope,
} from '@sentry/browser';
import { SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, getActiveSpan, spanToJSON, startInactiveSpan } from '@sentry/core';
import type { Integration, Span, Transaction, TransactionContext } from '@sentry/types';
import { logger, stripUrlQueryAndFragment, timestampInSeconds } from '@sentry/utils';
import type { Observable } from 'rxjs';
import { Subscription } from 'rxjs';
Expand All @@ -23,6 +28,8 @@ let instrumentationInitialized: boolean;
let stashedStartTransaction: (context: TransactionContext) => Transaction | undefined;
let stashedStartTransactionOnLocationChange: boolean;

let hooksBasedInstrumentation = false;

/**
* Creates routing instrumentation for Angular Router.
*/
Expand All @@ -49,6 +56,23 @@ export function routingInstrumentation(

export const instrumentAngularRouting = routingInstrumentation;

/**
* A custom BrowserTracing integration for Angular.
*/
export function browserTracingIntegration(
mydea marked this conversation as resolved.
Show resolved Hide resolved
options?: Parameters<typeof originalBrowserTracingIntegration>[0],
): Integration {
instrumentationInitialized = true;
hooksBasedInstrumentation = true;

return originalBrowserTracingIntegration({
...options,
instrumentPageLoad: true,
// We handle this manually
instrumentNavigation: false,
});
}
Comment on lines +70 to +76
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess one downside of this approach I just thought of is that there will be no navigation span/txn at all, if users don't register the TraceService. Previously they would have gotten the default browser one.

I think this is okay though - "incorrect" or incomplete SDK setup shouldn't be something we care too much about IMO. (Also, this is documented pretty clearly in docs)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I think I will refactor this based on what we talked about that we actually never overwrite the defaults like this but only do it via checking if something else registered a hook instead!


/**
* Grabs active transaction off scope.
*
Expand All @@ -74,7 +98,43 @@ export class TraceService implements OnDestroy {
return;
}

if (this._routingSpan) {
this._routingSpan.end();
this._routingSpan = null;
}

const strippedUrl = stripUrlQueryAndFragment(navigationEvent.url);

if (hooksBasedInstrumentation) {
if (!getActiveSpan()) {
browserTracingStartNavigationSpan({
name: strippedUrl,
op: 'navigation',
origin: 'auto.navigation.angular',
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
},
});
}

// eslint-disable-next-line deprecation/deprecation
this._routingSpan =
startInactiveSpan({
name: `${navigationEvent.url}`,
op: ANGULAR_ROUTING_OP,
origin: 'auto.ui.angular',
tags: {
'routing.instrumentation': '@sentry/angular',
url: strippedUrl,
...(navigationEvent.navigationTrigger && {
navigationTrigger: navigationEvent.navigationTrigger,
}),
},
}) || null;

return;
}

// eslint-disable-next-line deprecation/deprecation
let activeTransaction = getActiveTransaction();

Expand All @@ -90,9 +150,6 @@ export class TraceService implements OnDestroy {
}

if (activeTransaction) {
if (this._routingSpan) {
this._routingSpan.end();
}
// eslint-disable-next-line deprecation/deprecation
this._routingSpan = activeTransaction.startChild({
description: `${navigationEvent.url}`,
Expand Down
4 changes: 4 additions & 0 deletions packages/browser/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,13 @@ export {
} from '@sentry-internal/feedback';

export {
// eslint-disable-next-line deprecation/deprecation
BrowserTracing,
defaultRequestInstrumentationOptions,
instrumentOutgoingRequests,
browserTracingIntegration,
browserTracingStartNavigationSpan,
browserTracingStartPageLoadSpan,
} from '@sentry-internal/tracing';
export type { RequestInstrumentationOptions } from '@sentry-internal/tracing';
export {
Expand Down
13 changes: 13 additions & 0 deletions packages/core/src/baseclient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import type {
SessionAggregates,
Severity,
SeverityLevel,
StartSpanOptions,
Transaction,
TransactionEvent,
Transport,
Expand Down Expand Up @@ -481,6 +482,12 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
callback: (feedback: FeedbackEvent, options?: { includeReplay: boolean }) => void,
): void;

/** @inheritdoc */
public on(hook: 'startPageLoadSpan', callback: (options: StartSpanOptions) => void): void;

/** @inheritdoc */
public on(hook: 'startNavigationSpan', callback: (options: StartSpanOptions) => void): void;

/** @inheritdoc */
public on(hook: string, callback: unknown): void {
if (!this._hooks[hook]) {
Expand Down Expand Up @@ -521,6 +528,12 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
/** @inheritdoc */
public emit(hook: 'beforeSendFeedback', feedback: FeedbackEvent, options?: { includeReplay: boolean }): void;

/** @inheritdoc */
public emit(hook: 'startPageLoadSpan', options: StartSpanOptions): void;

/** @inheritdoc */
public emit(hook: 'startNavigationSpan', options: StartSpanOptions): void;

/** @inheritdoc */
public emit(hook: string, ...rest: unknown[]): void {
if (this._hooks[hook]) {
Expand Down
113 changes: 2 additions & 111 deletions packages/core/src/tracing/trace.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,5 @@
import type {
Instrumenter,
Primitive,
Scope,
Span,
SpanTimeInput,
TransactionContext,
TransactionMetadata,
} from '@sentry/types';
import type { SpanAttributes } from '@sentry/types';
import type { SpanOrigin } from '@sentry/types';
import type { TransactionSource } from '@sentry/types';
import type { Span, SpanTimeInput, StartSpanOptions, TransactionContext } from '@sentry/types';

import { dropUndefinedKeys, logger, tracingContextFromHeaders } from '@sentry/utils';

import { DEBUG_BUILD } from '../debug-build';
Expand All @@ -20,105 +10,6 @@ import { handleCallbackErrors } from '../utils/handleCallbackErrors';
import { hasTracingEnabled } from '../utils/hasTracingEnabled';
import { spanTimeInputToSeconds, spanToJSON } from '../utils/spanUtils';

interface StartSpanOptions extends TransactionContext {
/** A manually specified start time for the created `Span` object. */
startTime?: SpanTimeInput;

/** If defined, start this span off this scope instead off the current scope. */
scope?: Scope;

/** The name of the span. */
name: string;

/** An op for the span. This is a categorization for spans. */
op?: string;

/** The origin of the span - if it comes from auto instrumenation or manual instrumentation. */
origin?: SpanOrigin;

/** Attributes for the span. */
attributes?: SpanAttributes;

// All remaining fields are deprecated

/**
* @deprecated Manually set the end timestamp instead.
*/
trimEnd?: boolean;

/**
* @deprecated This cannot be set manually anymore.
*/
parentSampled?: boolean;

/**
* @deprecated Use attributes or set data on scopes instead.
*/
metadata?: Partial<TransactionMetadata>;

/**
* The name thingy.
* @deprecated Use `name` instead.
*/
description?: string;

/**
* @deprecated Use `span.setStatus()` instead.
*/
status?: string;

/**
* @deprecated Use `scope` instead.
*/
parentSpanId?: string;

/**
* @deprecated You cannot manually set the span to sampled anymore.
*/
sampled?: boolean;

/**
* @deprecated You cannot manually set the spanId anymore.
*/
spanId?: string;

/**
* @deprecated You cannot manually set the traceId anymore.
*/
traceId?: string;

/**
* @deprecated Use an attribute instead.
*/
source?: TransactionSource;

/**
* @deprecated Use attributes or set tags on the scope instead.
*/
tags?: { [key: string]: Primitive };

/**
* @deprecated Use attributes instead.
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
data?: { [key: string]: any };

/**
* @deprecated Use `startTime` instead.
*/
startTimestamp?: number;

/**
* @deprecated Use `span.end()` instead.
*/
endTimestamp?: number;

/**
* @deprecated You cannot set the instrumenter manually anymore.
*/
instrumenter?: Instrumenter;
}

/**
* Wraps a function with a transaction/span and finishes the span after the function is done.
*
Expand Down
Loading
Loading