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): Add hook for trace sampling function to SDK options #2820

Merged
merged 40 commits into from
Sep 9, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
df7181a
add tracessampler option; not yet tested
lobsterkatie Aug 14, 2020
fa72ceb
fix type error and some linting things
lobsterkatie Aug 14, 2020
290604d
respond to PR comments
lobsterkatie Aug 14, 2020
05a358e
don't return null from tracesSampler
lobsterkatie Aug 18, 2020
90854cc
random cleanup
lobsterkatie Aug 18, 2020
cbc216c
simplify implementation of extractrequestdata and add docstring
lobsterkatie Aug 19, 2020
85a0776
allow startTransaction to take a sampleContext argument which gets pa…
lobsterkatie Aug 19, 2020
4f1b484
factor out getActiveDomain as new function
lobsterkatie Aug 24, 2020
382320f
add getDefaultSampleContext method
lobsterkatie Aug 24, 2020
3587b64
rename internal startTransaction with leading underscore
lobsterkatie Aug 24, 2020
a7a48b5
move extractRequestData to @sentry/utils
lobsterkatie Aug 24, 2020
3855006
validate sample rate and clean up logging
lobsterkatie Aug 24, 2020
2a1d731
get location data from global object instead of window to account for…
lobsterkatie Aug 25, 2020
551afc4
create hybrid type for domains acting as carriers
lobsterkatie Aug 25, 2020
1453092
fix bug in parseRequest
lobsterkatie Aug 25, 2020
17512a0
break up utils.misc to fix circular imports
lobsterkatie Aug 26, 2020
8252e36
random cleanup
lobsterkatie Aug 26, 2020
c3e4b60
add tests for sampling
lobsterkatie Aug 26, 2020
4582354
fix circular package dependency
lobsterkatie Aug 26, 2020
c9d77ec
fix gatsby test
lobsterkatie Aug 26, 2020
e784f4b
fix linting errors
lobsterkatie Sep 1, 2020
669428c
fix type issues on global object
lobsterkatie Sep 1, 2020
6fb10c3
add domain to extensions interface and active to domain interface
lobsterkatie Sep 1, 2020
2853c8f
move tracing enablement check to tracing package
lobsterkatie Sep 1, 2020
36b8b4f
remove end markers on describe blocks
lobsterkatie Sep 1, 2020
101f9d0
remove note about weird mocking hack
lobsterkatie Sep 1, 2020
85deeb6
add specific types for default and custom sample context data
lobsterkatie Sep 2, 2020
64fde64
don't pass custom context to startIdleTransaction since it's an inter…
lobsterkatie Sep 2, 2020
573fe73
move tracesSampleRate and tracesSampler to be closer in the Options t…
lobsterkatie Sep 2, 2020
b98bded
meld default data into main SampleContext type so it appears in type …
lobsterkatie Sep 2, 2020
9c6f0cb
add tracesSampler example to gatsby readme
lobsterkatie Sep 2, 2020
fd4d48c
random cleanup
lobsterkatie Sep 2, 2020
7b47a06
add parent's sampling decision to defaultSampleContext
lobsterkatie Sep 3, 2020
be6abc8
make tracesSampler able to return a boolean and make it override samp…
lobsterkatie Sep 3, 2020
69e49e3
protect against NaN values for sample rate
lobsterkatie Sep 3, 2020
cbaecb3
remove claim that we're fixing a race condition by copying location o…
lobsterkatie Sep 3, 2020
ccba78a
s/sampleContext/samplingContext
lobsterkatie Sep 3, 2020
d6fd44a
pass transaction context in sampling context
lobsterkatie Sep 7, 2020
0710860
include parentSampled as separate entry in transaction context
lobsterkatie Sep 7, 2020
8572b08
refactor toTraceparent into extractTraceparentData
lobsterkatie Sep 7, 2020
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
31 changes: 27 additions & 4 deletions packages/gatsby/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ To automatically capture the `release` value on Vercel you will need to register

## Sentry Performance

To enable Tracing support, supply the `tracesSampleRate` to the options and make sure you have installed the `@sentry/tracing` package. This will also turn on the `BrowserTracing` integration for automatic instrumentation of the browser.
To enable tracing, supply either `tracesSampleRate` or `tracesSampler` to the options and make sure you have installed the `@sentry/tracing` package. This will also turn on the `BrowserTracing` integration for automatic instrumentation of pageloads and navigations.

```javascript
{
Expand All @@ -49,8 +49,31 @@ To enable Tracing support, supply the `tracesSampleRate` to the options and make
{
resolve: "@sentry/gatsby",
options: {
dsn: process.env.SENTRY_DSN, // this is the default
tracesSampleRate: 1, // this is just to test, you should lower this in production
dsn: process.env.SENTRY_DSN, // this is the default

// A rate of 1 means all traces will be sent, so it's good for testing.
// In production, you'll likely want to either choose a lower rate or use `tracesSampler` instead (see below).
tracesSampleRate: 1,

// Alternatively:
tracesSampler: samplingContext => {
// Examine provided context data (along with anything in the global namespace) to decide the sample rate
// for this transaction.
// Can return 0 to drop the transaction entirely.

if ("...") {
return 0.5 // These are important - take a big sample
}
else if ("...") {
return 0.01 // These are less important or happen much more frequently - only take 1% of them
}
else if ("...") {
return 0 // These aren't something worth tracking - drop all transactions like this
}
else {
return 0.1 // Default sample rate
}
}
}
},
// ...
Expand All @@ -68,7 +91,7 @@ If you want to supply options to the `BrowserTracing` integration, use the `brow
resolve: "@sentry/gatsby",
options: {
dsn: process.env.SENTRY_DSN, // this is the default
tracesSampleRate: 1, // this is just to test, you should lower this in production
tracesSampleRate: 1, // or tracesSampler (see above)
browserTracingOptions: {
// disable creating spans for XHR requests
traceXHR: false,
Expand Down
4 changes: 1 addition & 3 deletions packages/gatsby/gatsby-browser.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,9 @@ exports.onClientEntry = function(_, pluginParams) {
return;
}

const tracesSampleRate = pluginParams.tracesSampleRate !== undefined ? pluginParams.tracesSampleRate : 0;
const integrations = [...(pluginParams.integrations || [])];

if (tracesSampleRate && !integrations.some(ele => ele.name === 'BrowserTracing')) {
if (Tracing.hasTracingEnabled(pluginParams) && !integrations.some(ele => ele.name === 'BrowserTracing')) {
integrations.push(new Tracing.Integrations.BrowserTracing(pluginParams.browserTracingOptions));
}

Expand All @@ -22,7 +21,6 @@ exports.onClientEntry = function(_, pluginParams) {
// eslint-disable-next-line no-undef
dsn: __SENTRY_DSN__,
...pluginParams,
tracesSampleRate,
integrations,
});

Expand Down
21 changes: 20 additions & 1 deletion packages/gatsby/test/gatsby-browser.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ describe('onClientEntry', () => {
environment: process.env.NODE_ENV,
integrations: [],
release: (global as any).__SENTRY_RELEASE__,
tracesSampleRate: 0,
});
});

Expand Down Expand Up @@ -100,6 +99,16 @@ describe('onClientEntry', () => {
);
});

it('sets a tracesSampler if defined as option', () => {
const tracesSampler = jest.fn();
onClientEntry(undefined, { tracesSampler });
expect(sentryInit).toHaveBeenLastCalledWith(
expect.objectContaining({
tracesSampler,
}),
);
});

it('adds `BrowserTracing` integration if tracesSampleRate is defined', () => {
onClientEntry(undefined, { tracesSampleRate: 0.5 });
expect(sentryInit).toHaveBeenLastCalledWith(
Expand All @@ -109,6 +118,16 @@ describe('onClientEntry', () => {
);
});

it('adds `BrowserTracing` integration if tracesSampler is defined', () => {
const tracesSampler = jest.fn();
onClientEntry(undefined, { tracesSampler });
expect(sentryInit).toHaveBeenLastCalledWith(
expect.objectContaining({
integrations: [expect.objectContaining({ name: 'BrowserTracing' })],
}),
);
});

it('only defines a single `BrowserTracing` integration', () => {
const Tracing = jest.requireActual('@sentry/tracing');
const integrations = [new Tracing.Integrations.BrowserTracing()];
Expand Down
29 changes: 16 additions & 13 deletions packages/hub/src/hub.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
Breadcrumb,
BreadcrumbHint,
Client,
CustomSamplingContext,
Event,
EventHint,
Extra,
Expand All @@ -19,7 +20,7 @@ import {
} from '@sentry/types';
import { consoleSandbox, getGlobalObject, isNodeEnv, logger, timestampWithMs, uuid4 } from '@sentry/utils';

import { Carrier, Layer } from './interfaces';
import { Carrier, DomainAsCarrier, Layer } from './interfaces';
import { Scope } from './scope';

/**
Expand Down Expand Up @@ -369,8 +370,8 @@ export class Hub implements HubInterface {
/**
* @inheritDoc
*/
public startTransaction(context: TransactionContext): Transaction {
return this._callExtensionMethod('startTransaction', context);
public startTransaction(context: TransactionContext, customSamplingContext?: CustomSamplingContext): Transaction {
return this._callExtensionMethod('startTransaction', context, customSamplingContext);
}

/**
Expand Down Expand Up @@ -456,22 +457,24 @@ export function getCurrentHub(): Hub {
return getHubFromCarrier(registry);
}

/**
* Returns the active domain, if one exists
*
* @returns The domain, or undefined if there is no active domain
*/
export function getActiveDomain(): DomainAsCarrier | undefined {
const sentry = getMainCarrier().__SENTRY__;

return sentry && sentry.extensions && sentry.extensions.domain && sentry.extensions.domain.active;
}

/**
* Try to read the hub from an active domain, and fallback to the registry if one doesn't exist
* @returns discovered hub
*/
function getHubFromActiveDomain(registry: Carrier): Hub {
try {
const property = 'domain';
const carrier = getMainCarrier();
const sentry = carrier.__SENTRY__;
if (!sentry || !sentry.extensions || !sentry.extensions[property]) {
return getHubFromCarrier(registry);
}
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const domain = sentry.extensions[property] as any;
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
const activeDomain = domain.active;
const activeDomain = getActiveDomain();

// If there's no active domain, just return global hub
if (!activeDomain) {
Expand Down
12 changes: 10 additions & 2 deletions packages/hub/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
export { Carrier, Layer } from './interfaces';
export { Carrier, DomainAsCarrier, Layer } from './interfaces';
export { addGlobalEventProcessor, Scope } from './scope';
export { getCurrentHub, getHubFromCarrier, getMainCarrier, Hub, makeMain, setHubOnCarrier } from './hub';
export {
getActiveDomain,
getCurrentHub,
getHubFromCarrier,
getMainCarrier,
Hub,
makeMain,
setHubOnCarrier,
} from './hub';
21 changes: 18 additions & 3 deletions packages/hub/src/interfaces.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Client } from '@sentry/types';
import * as domain from 'domain';

import { Hub } from './hub';
import { Scope } from './scope';
Expand All @@ -20,9 +21,23 @@ export interface Carrier {
__SENTRY__?: {
hub?: Hub;
/**
* These are extension methods for the hub, the current instance of the hub will be bound to it
* Extra Hub properties injected by various SDKs
*/
// eslint-disable-next-line @typescript-eslint/ban-types
extensions?: { [key: string]: Function };
extensions?: {
/** Hack to prevent bundlers from breaking our usage of the domain package in the cross-platform Hub package */
domain?: typeof domain & {
/**
* The currently active domain. This is part of the domain package, but for some reason not declared in the
* package's typedef.
*/
active?: domain.Domain;
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't be using @types/node in an isomorphic package.

};
} & {
/** Extension methods for the hub, which are bound to the current Hub instance */
// eslint-disable-next-line @typescript-eslint/ban-types
[key: string]: Function;
};
Comment on lines +26 to +39
Copy link
Member

Choose a reason for hiding this comment

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

H: Are we sure that doesn't break in Browser environments?

Copy link
Member Author

Choose a reason for hiding this comment

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

I will check again, for sure, but the change here isn't a behavior change but merely a TS one. We've always put the domain there, it's just that before we didn't include it in the type so we had to do a lot of eslint-disable @no-unsafe-member-access and eslint-disable @no-explicit-any. My understanding is that TS stuff all gets stripped?

Anyway, will check and update here.

Copy link
Member Author

Choose a reason for hiding this comment

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

UPDATE: Seems to build just fine, with no special accommodation in my set up.

Choose a reason for hiding this comment

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

@lobsterkatie It seems like these changes broke @sentry/browser for TS users by requiring node types 😕 issue: #2927

Copy link
Contributor

Choose a reason for hiding this comment

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

See #3050 for a fix.

};
}

export interface DomainAsCarrier extends domain.Domain, Carrier {}
28 changes: 16 additions & 12 deletions packages/minimal/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { getCurrentHub, Hub, Scope } from '@sentry/hub';
import {
Breadcrumb,
CaptureContext,
CustomSamplingContext,
Event,
Extra,
Extras,
Expand Down Expand Up @@ -190,22 +191,25 @@ export function _callOnClient(method: string, ...args: any[]): void {
}

/**
* Starts a new `Transaction` and returns it. This is the entry point to manual
* tracing instrumentation.
* Starts a new `Transaction` and returns it. This is the entry point to manual tracing instrumentation.
*
* A tree structure can be built by adding child spans to the transaction, and
* child spans to other spans. To start a new child span within the transaction
* or any span, call the respective `.startChild()` method.
* A tree structure can be built by adding child spans to the transaction, and child spans to other spans. To start a
* new child span within the transaction or any span, call the respective `.startChild()` method.
*
* Every child span must be finished before the transaction is finished,
* otherwise the unfinished spans are discarded.
* Every child span must be finished before the transaction is finished, otherwise the unfinished spans are discarded.
*
* The transaction must be finished with a call to its `.finish()` method, at
* which point the transaction with all its finished child spans will be sent to
* Sentry.
* The transaction must be finished with a call to its `.finish()` method, at which point the transaction with all its
* finished child spans will be sent to Sentry.
*
* @param context Properties of the new `Transaction`.
* @param customSamplingContext Information given to the transaction sampling function (along with context-dependent
* default values). See {@link Options.tracesSampler}.
*
* @returns The transaction which was just started
*/
export function startTransaction(context: TransactionContext): Transaction {
return callOnHub('startTransaction', { ...context });
export function startTransaction(
context: TransactionContext,
customSamplingContext?: CustomSamplingContext,
): Transaction {
return callOnHub('startTransaction', { ...context }, customSamplingContext);
}
Loading