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.

Sign up for GitHub

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

Jump to bottom

fix(sampling): Remove stray sampling data tags #3197

Merged
merged 6 commits into from
Feb 1, 2021
Merged

fix(sampling): Remove stray sampling data tags #3197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d8c2181
add transaction sampling to DebugMeta type
lobsterkatie Jan 22, 2021
894566f
add metadata property to Transaction class
lobsterkatie Jan 22, 2021
f6cda3e
store sampling data in metadata property rather than tags
lobsterkatie Jan 22, 2021
31a7718
pull sampling data for envelope headers from debug_meta rather than tags
lobsterkatie Jan 22, 2021
623584d
fix and clean up tests
lobsterkatie Jan 22, 2021
c69a371
use named interface for transaction metadata
lobsterkatie Jan 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 8 additions & 4 deletions packages/core/src/request.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,14 +51,18 @@ export function sessionToSentryRequest(session: Session, api: API): SentryReques

/** Creates a SentryRequest from an event. */
export function eventToSentryRequest(event: Event, api: API): SentryRequest {
// since JS has no Object.prototype.pop()
const { __sentry_samplingMethod: samplingMethod, __sentry_sampleRate: sampleRate, ...otherTags } = event.tags || {};
event.tags = otherTags;

const sdkInfo = getSdkMetadataForEnvelopeHeader(api);
const eventType = event.type || 'event';
const useEnvelope = eventType === 'transaction';

const { transactionSampling, ...metadata } = event.debug_meta || {};
const { method: samplingMethod, rate: sampleRate } = transactionSampling || {};
if (Object.keys(metadata).length === 0) {
lobsterkatie marked this conversation as resolved.
Show resolved Hide resolved
delete event.debug_meta;
} else {
event.debug_meta = metadata;
}

const req: SentryRequest = {
body: JSON.stringify(sdkInfo ? enhanceEventWithSdkInfo(event, api.metadata.sdk) : event),
type: eventType,
Expand Down
127 changes: 65 additions & 62 deletions packages/core/test/lib/request.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,30 @@
import { Event, TransactionSamplingMethod } from '@sentry/types';
import { DebugMeta, Event, SentryRequest, TransactionSamplingMethod } from '@sentry/types';

import { API } from '../../src/api';
import { eventToSentryRequest } from '../../src/request';

describe('eventToSentryRequest', () => {
let api: API;
function parseEnvelopeRequest(request: SentryRequest): any {
const [envelopeHeaderString, itemHeaderString, eventString] = request.body.split('\n');

return {
envelopeHeader: JSON.parse(envelopeHeaderString),
itemHeader: JSON.parse(itemHeaderString),
event: JSON.parse(eventString),
};
}

const api = new API('https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012', {
sdk: {
integrations: ['AWSLambda'],
name: 'sentry.javascript.browser',
version: `12.31.12`,
packages: [{ name: 'npm:@sentry/browser', version: `12.31.12` }],
},
});
let event: Event;

beforeEach(() => {
api = new API('https://dogsarebadatkeepingsecrets@squirrelchasers.ingest.sentry.io/12312012', {
sdk: {
integrations: ['AWSLambda'],
name: 'sentry.javascript.browser',
version: `12.31.12`,
packages: [{ name: 'npm:@sentry/browser', version: `6.6.6` }],
},
});
event = {
contexts: { trace: { trace_id: '1231201211212012', span_id: '12261980', op: 'pageload' } },
environment: 'dogpark',
Expand All @@ -28,68 +37,63 @@ describe('eventToSentryRequest', () => {
};
});

[
{ method: TransactionSamplingMethod.Rate, rate: '0.1121', dog: 'Charlie' },
{ method: TransactionSamplingMethod.Sampler, rate: '0.1231', dog: 'Maisey' },
{ method: TransactionSamplingMethod.Inheritance, dog: 'Cory' },
{ method: TransactionSamplingMethod.Explicit, dog: 'Bodhi' },

// this shouldn't ever happen (at least the method should always be included in tags), but good to know that things
// won't blow up if it does
{ dog: 'Lucy' },
].forEach(({ method, rate, dog }) => {
it(`adds transaction sampling information to item header - ${method}, ${rate}, ${dog}`, () => {
// TODO kmclb - once tag types are loosened, don't need to cast to string here
event.tags = { __sentry_samplingMethod: String(method), __sentry_sampleRate: String(rate), dog };

const result = eventToSentryRequest(event, api);

const [envelopeHeaderString, itemHeaderString, eventString] = result.body.split('\n');

const envelope = {
envelopeHeader: JSON.parse(envelopeHeaderString),
itemHeader: JSON.parse(itemHeaderString),
event: JSON.parse(eventString),
};

// the right stuff is added to the item header
expect(envelope.itemHeader).toEqual({
type: 'transaction',
// TODO kmclb - once tag types are loosened, don't need to cast to string here
sample_rates: [{ id: String(method), rate: String(rate) }],
});

// show that it pops the right tags and leaves the rest alone
expect('__sentry_samplingMethod' in envelope.event.tags).toBe(false);
expect('__sentry_sampleRate' in envelope.event.tags).toBe(false);
expect('dog' in envelope.event.tags).toBe(true);
});
it(`adds transaction sampling information to item header`, () => {
event.debug_meta = { transactionSampling: { method: TransactionSamplingMethod.Rate, rate: 0.1121 } };

const result = eventToSentryRequest(event, api);
const envelope = parseEnvelopeRequest(result);

expect(envelope.itemHeader).toEqual(
expect.objectContaining({
sample_rates: [{ id: TransactionSamplingMethod.Rate, rate: 0.1121 }],
}),
);
});

it('adds sdk info to envelope header', () => {
it('removes transaction sampling information (and only that) from debug_meta', () => {
event.debug_meta = {
transactionSampling: { method: TransactionSamplingMethod.Sampler, rate: 0.1121 },
dog: 'Charlie',
} as DebugMeta;

const result = eventToSentryRequest(event, api);
const envelope = parseEnvelopeRequest(result);

const envelopeHeaderString = result.body.split('\n')[0];
const parsedHeader = JSON.parse(envelopeHeaderString);
expect('transactionSampling' in envelope.event.debug_meta).toBe(false);
expect('dog' in envelope.event.debug_meta).toBe(true);
});

expect(parsedHeader).toEqual(
it('removes debug_meta entirely if it ends up empty', () => {
event.debug_meta = {
transactionSampling: { method: TransactionSamplingMethod.Rate, rate: 0.1121 },
} as DebugMeta;

const result = eventToSentryRequest(event, api);
const envelope = parseEnvelopeRequest(result);

expect('debug_meta' in envelope.event).toBe(false);
});

it('adds sdk info to envelope header', () => {
const result = eventToSentryRequest(event, api);
const envelope = parseEnvelopeRequest(result);

expect(envelope.envelopeHeader).toEqual(
expect.objectContaining({ sdk: { name: 'sentry.javascript.browser', version: '12.31.12' } }),
);
});

it('adds sdk info to event body', () => {
const result = eventToSentryRequest(event, api);
const envelope = parseEnvelopeRequest(result);

const eventString = result.body.split('\n')[2];
const parsedEvent = JSON.parse(eventString);

expect(parsedEvent).toEqual(
expect(envelope.event).toEqual(
expect.objectContaining({
sdk: {
integrations: ['AWSLambda'],
name: 'sentry.javascript.browser',
version: `12.31.12`,
packages: [{ name: 'npm:@sentry/browser', version: `6.6.6` }],
packages: [{ name: 'npm:@sentry/browser', version: `12.31.12` }],
},
}),
);
Expand All @@ -99,22 +103,21 @@ describe('eventToSentryRequest', () => {
event.sdk = {
integrations: ['Clojure'],
name: 'foo',
packages: [{ name: 'npm:@sentry/clj', version: `6.6.6` }],
packages: [{ name: 'npm:@sentry/clj', version: `12.31.12` }],
version: '1337',
};
const result = eventToSentryRequest(event, api);

const eventString = result.body.split('\n')[2];
const parsedEvent = JSON.parse(eventString);
const result = eventToSentryRequest(event, api);
const envelope = parseEnvelopeRequest(result);

expect(parsedEvent).toEqual(
expect(envelope.event).toEqual(
expect.objectContaining({
sdk: {
integrations: ['Clojure', 'AWSLambda'],
name: 'foo',
packages: [
{ name: 'npm:@sentry/clj', version: `6.6.6` },
{ name: 'npm:@sentry/browser', version: `6.6.6` },
{ name: 'npm:@sentry/clj', version: `12.31.12` },
{ name: 'npm:@sentry/browser', version: `12.31.12` },
],
version: '1337',
},
Expand Down
36 changes: 20 additions & 16 deletions packages/tracing/src/hubextensions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ function sample<T extends Transaction>(hub: Hub, transaction: T, samplingContext

// if the user has forced a sampling decision by passing a `sampled` value in their transaction context, go with that
if (transaction.sampled !== undefined) {
transaction.tags = { ...transaction.tags, __sentry_samplingMethod: TransactionSamplingMethod.Explicit };
transaction.setMetadata({
transactionSampling: { method: TransactionSamplingMethod.Explicit },
});
return transaction;
}

Expand All @@ -54,25 +56,27 @@ function sample<T extends Transaction>(hub: Hub, transaction: T, samplingContext
let sampleRate;
if (typeof options.tracesSampler === 'function') {
sampleRate = options.tracesSampler(samplingContext);
// cast the rate to a number first in case it's a boolean
transaction.tags = {
...transaction.tags,
__sentry_samplingMethod: TransactionSamplingMethod.Sampler,
// TODO kmclb - once tag types are loosened, don't need to cast to string here
__sentry_sampleRate: String(Number(sampleRate)),
};
transaction.setMetadata({
transactionSampling: {
method: TransactionSamplingMethod.Sampler,
// cast to number in case it's a boolean
rate: Number(sampleRate),
Copy link
Member Author

Choose a reason for hiding this comment

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

@jan-auer Once consequence of this change is that rates will come through as numbers rather than strings. (When I did the original PR, tags had to be strings, which is why I stringified the value in the first place.) LMK if that's not good from a relay perspective and I can turn them back into strings.

Copy link
Member

@jan-auer jan-auer Jan 29, 2021 •
edited

Choose a reason for hiding this comment

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

Is this what you put into the sample_rates item header? The schema in there is:

type SampleRate = {
  id: String,
  rate: Number,
}

If this goes into the header, let's rather keep it consistent to avoid mistakes please. As for the rate, that should be a number all along.

},
});
} else if (samplingContext.parentSampled !== undefined) {
sampleRate = samplingContext.parentSampled;
transaction.tags = { ...transaction.tags, __sentry_samplingMethod: TransactionSamplingMethod.Inheritance };
transaction.setMetadata({
transactionSampling: { method: TransactionSamplingMethod.Inheritance },
});
} else {
sampleRate = options.tracesSampleRate;
// cast the rate to a number first in case it's a boolean
transaction.tags = {
...transaction.tags,
__sentry_samplingMethod: TransactionSamplingMethod.Rate,
// TODO kmclb - once tag types are loosened, don't need to cast to string here
__sentry_sampleRate: String(Number(sampleRate)),
};
transaction.setMetadata({
transactionSampling: {
method: TransactionSamplingMethod.Rate,
// cast to number in case it's a boolean
rate: Number(sampleRate),
},
});
}

// Since this is coming from the user (or from a function provided by the user), who knows what we might get. (The
Expand Down
16 changes: 16 additions & 0 deletions packages/tracing/src/transaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,16 @@ import { dropUndefinedKeys, isInstanceOf, logger } from '@sentry/utils';

import { Span as SpanClass, SpanRecorder } from './span';

interface TransactionMetadata {
transactionSampling?: { [key: string]: string | number };
}

/** JSDoc */
export class Transaction extends SpanClass implements TransactionInterface {
public name: string;

private _metadata: TransactionMetadata = {};

private _measurements: Measurements = {};

/**
Expand Down Expand Up @@ -64,6 +71,14 @@ export class Transaction extends SpanClass implements TransactionInterface {
this._measurements = { ...measurements };
}

/**
* Set metadata for this transaction.
* @hidden
*/
public setMetadata(newMetadata: TransactionMetadata): void {
this._metadata = { ...this._metadata, ...newMetadata };
}

/**
* @inheritDoc
*/
Expand Down Expand Up @@ -108,6 +123,7 @@ export class Transaction extends SpanClass implements TransactionInterface {
timestamp: this.endTimestamp,
transaction: this.name,
type: 'transaction',
debug_meta: this._metadata,
};

const hasMeasurements = Object.keys(this._measurements).length > 0;
Expand Down
33 changes: 20 additions & 13 deletions packages/tracing/test/hub.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,20 @@
import { BrowserClient } from '@sentry/browser';
import { Hub } from '@sentry/hub';
import * as hubModule from '@sentry/hub';
import { TransactionSamplingMethod } from '@sentry/types';
import * as utilsModule from '@sentry/utils'; // for mocking
import { logger } from '@sentry/utils';

import { BrowserTracing } from '../src/browser/browsertracing';
import { addExtensionMethods } from '../src/hubextensions';
import { Transaction } from '../src/transaction';
import { extractTraceparentData, TRACEPARENT_REGEXP } from '../src/utils';
import { addDOMPropertiesToGlobal, getSymbolObjectKeyByName } from './testutils';

addExtensionMethods();

const mathRandom = jest.spyOn(Math, 'random');
jest.spyOn(Transaction.prototype, 'setMetadata');

// we have to add things into the real global object (rather than mocking the return value of getGlobalObject) because
// there are modules which call getGlobalObject as they load, which is seemingly too early for jest to intervene
Expand All @@ -26,7 +29,7 @@ describe('Hub', () => {
});

afterEach(() => {
jest.restoreAllMocks();
jest.clearAllMocks();
jest.useRealTimers();
});

Expand Down Expand Up @@ -184,35 +187,39 @@ describe('Hub', () => {
it('should record sampling method when sampling decision is explicitly set', () => {
const tracesSampler = jest.fn().mockReturnValue(0.1121);
const hub = new Hub(new BrowserClient({ tracesSampler }));
const transaction = hub.startTransaction({ name: 'dogpark', sampled: true });
hub.startTransaction({ name: 'dogpark', sampled: true });

expect(transaction.tags).toEqual(expect.objectContaining({ __sentry_samplingMethod: 'explicitly_set' }));
expect(Transaction.prototype.setMetadata).toHaveBeenCalledWith({
transactionSampling: { method: TransactionSamplingMethod.Explicit },
});
});

it('should record sampling method and rate when sampling decision comes from tracesSampler', () => {
const tracesSampler = jest.fn().mockReturnValue(0.1121);
const hub = new Hub(new BrowserClient({ tracesSampler }));
const transaction = hub.startTransaction({ name: 'dogpark' });
hub.startTransaction({ name: 'dogpark' });

expect(transaction.tags).toEqual(
expect.objectContaining({ __sentry_samplingMethod: 'client_sampler', __sentry_sampleRate: '0.1121' }),
);
expect(Transaction.prototype.setMetadata).toHaveBeenCalledWith({
transactionSampling: { method: TransactionSamplingMethod.Sampler, rate: 0.1121 },
});
});

it('should record sampling method when sampling decision is inherited', () => {
const hub = new Hub(new BrowserClient({ tracesSampleRate: 0.1121 }));
const transaction = hub.startTransaction({ name: 'dogpark', parentSampled: true });
hub.startTransaction({ name: 'dogpark', parentSampled: true });

expect(transaction.tags).toEqual(expect.objectContaining({ __sentry_samplingMethod: 'inheritance' }));
expect(Transaction.prototype.setMetadata).toHaveBeenCalledWith({
transactionSampling: { method: TransactionSamplingMethod.Inheritance },
});
});

it('should record sampling method and rate when sampling decision comes from traceSampleRate', () => {
const hub = new Hub(new BrowserClient({ tracesSampleRate: 0.1121 }));
const transaction = hub.startTransaction({ name: 'dogpark' });
hub.startTransaction({ name: 'dogpark' });

expect(transaction.tags).toEqual(
expect.objectContaining({ __sentry_samplingMethod: 'client_rate', __sentry_sampleRate: '0.1121' }),
);
expect(Transaction.prototype.setMetadata).toHaveBeenCalledWith({
transactionSampling: { method: TransactionSamplingMethod.Rate, rate: 0.1121 },
});
});
});

Expand Down
1 change: 1 addition & 0 deletions packages/types/src/debugMeta.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
**/
export interface DebugMeta {
images?: Array<DebugImage>;
transactionSampling?: { rate?: number; method?: string };
Copy link
Contributor

Choose a reason for hiding this comment

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

If we call it rate and id in the protocol, couldn't we unify it here as well? It'd make some object deconstruction easier.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would make object deconstruction easier. I didn't do it because id is opaque. What's it the id of? Unless you feel strongly I'd rather keep it as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

I totally agree, but why is Relay using it in the first place? :P

Copy link
Member Author

Choose a reason for hiding this comment

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

You'll have to take that up with @jan-auer 😛

Copy link
Member

Choose a reason for hiding this comment

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

See also #3197 (comment). Relay doesn't use this field, but it also sample in its own way. The id field identifies not only the method but in case of Relay also the rule which applied.

Ultimately, this all ends up in Sentry and in our data pipeline.

}

/**
Expand Down