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[apm]: Use Performance API #2474

Merged
merged 9 commits into from
Mar 9, 2020
Merged
Show file tree
Hide file tree
Changes from 4 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
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

- "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott

- [apm] feat: Make use of the `performance` browser API to provide better instrumentation (#2474)

## 5.13.2

- [apm] feat: Add `discardBackgroundSpans` to discard background spans by default
Expand Down
205 changes: 203 additions & 2 deletions packages/apm/src/integrations/tracing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,12 +132,17 @@ export class Tracing implements Integration {

private readonly _emitOptionsWarning: boolean = false;

private static _performanceCursor: number = 0;

/**
* Constructor for Tracing
*
* @param _options TracingOptions
*/
public constructor(_options?: Partial<TracingOptions>) {
if (global.performance) {
global.performance.mark('sentry-tracing-init');
}
const defaults = {
discardBackgroundSpans: true,
idleTimeout: 500,
Expand Down Expand Up @@ -236,6 +241,7 @@ export class Tracing implements Integration {
event.timestamp - event.start_timestamp < 0);

if (Tracing.options.maxTransactionDuration !== 0 && event.type === 'transaction' && isOutdatedTransaction) {
logger.log('[Tracing] Discarded transaction since it maxed out maxTransactionDuration');
return null;
}
}
Expand Down Expand Up @@ -295,6 +301,7 @@ export class Tracing implements Integration {
);

Tracing._activeTransaction = span;
Tracing._addOffsetToSpan(`idleTransactionStarted-${Tracing._currentIndex}`, span as SpanClass);

// We need to do this workaround here and not use configureScope
// Reason being at the time we start the inital transaction we do not have a client bound on the hub yet
Expand Down Expand Up @@ -336,11 +343,171 @@ export class Tracing implements Integration {
const active = Tracing._activeTransaction as SpanClass;
if (active) {
logger.log('[Tracing] finishIdleTransaction', active.transaction);
Tracing._addPerformanceEntries(active);
// true = use timestamp of last span
active.finish(true);
}
}

/**
* This uses `performance.getEntries()` to add additional spans to the active transaction.
* Also, we update our timings since we consider the timings in this API to be more correct than our manual
* measurements.
*
* @param transactionSpan The transaction span
Copy link
Contributor

@kamilogorek kamilogorek Mar 9, 2020

Choose a reason for hiding this comment

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

revolver ocelot

— revolver ocelot

*/
private static _addPerformanceEntries(transactionSpan: SpanClass): void {
let navigationOffset = 0;
if (
transactionSpan.op === 'navigation' &&
transactionSpan.data &&
typeof transactionSpan.data.offset === 'number'
) {
navigationOffset = transactionSpan.data.offset;
}
// tslint:disable-next-line: completed-docs
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can just disable it in a whole file instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem seems to be TS lint here, even if I write docs it still complains.

function addSpan(span: SpanClass): void {
HazAT marked this conversation as resolved.
Show resolved Hide resolved
// tslint:disable-next-line: no-non-null-assertion
transactionSpan.spanRecorder!.finishSpan(span);
}

// tslint:disable-next-line: completed-docs
function addPerformanceNavigationTiming(parent: SpanClass, entry: any, event: string): void {
const span = parent.child({
description: event,
op: 'browser',
});
// tslint:disable: no-unsafe-any
span.startTimestamp = parent.startTimestamp + Tracing._msToSec(entry[`${event}Start`] as number);
Copy link
Contributor

@kamilogorek kamilogorek Mar 9, 2020

Choose a reason for hiding this comment

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

It can be written as

span.timestamp = span.startTimestamp = parent.startTimestamp + Tracing._msToSec(entry[`${event}Start`] as number);

Copy link
Member Author

@HazAT HazAT Mar 9, 2020

Choose a reason for hiding this comment

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

Nope,

Tracing._msToSec(entry[`${event}Start`]
Tracing._msToSec(entry[`${event}End`]

span.timestamp = parent.startTimestamp + Tracing._msToSec(entry[`${event}End`] as number);
// tslint:enable: no-unsafe-any
addSpan(span);
}

// tslint:disable-next-line: completed-docs
function addRequest(parent: SpanClass, entry: any): void {
const request = parent.child({
description: 'request',
op: 'browser',
});
// tslint:disable: no-unsafe-any
request.startTimestamp = parent.startTimestamp + Tracing._msToSec(entry.requestStart as number);
HazAT marked this conversation as resolved.
Show resolved Hide resolved
request.timestamp = parent.startTimestamp + Tracing._msToSec(entry.responseEnd as number);
// tslint:enable: no-unsafe-any
addSpan(request);
const response = parent.child({
description: 'response',
op: 'browser',
});
// tslint:disable: no-unsafe-any
response.startTimestamp = parent.startTimestamp + Tracing._msToSec(entry.responseStart as number);
response.timestamp = parent.startTimestamp + Tracing._msToSec(entry.responseEnd as number);
// tslint:enable: no-unsafe-any
addSpan(response);
}

let entryScriptSrc: string | undefined;

if (global.document) {
// tslint:disable-next-line: prefer-for-of
for (let i = 0; i < document.scripts.length; i++) {
// We go through all scripts on the page and look for 'data-entry'
// We remember the name and measure the time between this script finished loading and
// our mark 'sentry-tracing-init'
if (document.scripts[i].getAttribute('data-entry') === 'true') {
Copy link
Member

Choose a reason for hiding this comment

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

Does the user need to manually indicate the data-entry attribute?

Copy link
Member Author

@HazAT HazAT Mar 6, 2020

Choose a reason for hiding this comment

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

Yes, otherwise it's hard to determine which is the "entry script" of your application.
If that data-entry is not there, it's just missing the script.evaluation span.
Also added an internal note in the description of the PR so we don't forget to document this if we think it makes sense.

HazAT marked this conversation as resolved.
Show resolved Hide resolved
entryScriptSrc = document.scripts[i].src;
break;
}
}
}

let entryScriptStartEndTime: number | undefined;
let tracingInitMarkStartTime: number | undefined;

// tslint:disable: no-unsafe-any
performance
.getEntries()
.slice(Tracing._performanceCursor)
.forEach((entry: any) => {
const startTime = Tracing._msToSec(entry.startTime as number);
const duration = Tracing._msToSec(entry.duration as number);

switch (entry.entryType) {
case 'navigation':
addPerformanceNavigationTiming(transactionSpan, entry, 'unloadEvent');
addPerformanceNavigationTiming(transactionSpan, entry, 'domContentLoadedEvent');
addPerformanceNavigationTiming(transactionSpan, entry, 'loadEvent');
addPerformanceNavigationTiming(transactionSpan, entry, 'connect');
addPerformanceNavigationTiming(transactionSpan, entry, 'domainLookup');
addRequest(transactionSpan, entry);
break;
case 'mark':
case 'paint':
case 'measure':
const mark = transactionSpan.child({
description: `${entry.entryType} ${entry.name}`,
op: 'mark',
});
mark.startTimestamp = transactionSpan.startTimestamp + startTime - navigationOffset;
mark.timestamp = mark.startTimestamp + duration;
if (tracingInitMarkStartTime === undefined && entry.name === 'sentry-tracing-init') {
tracingInitMarkStartTime = mark.startTimestamp;
}
addSpan(mark);
break;
case 'resource':
const resourceName = entry.name.replace(window.location.origin, '');
if (entry.initiatorType === 'xmlhttprequest' || entry.initiatorType === 'fetch') {
// We need to update existing spans with new timing info
// tslint:disable-next-line: no-non-null-assertion
transactionSpan.spanRecorder!.finishedSpans.map((finishedSpan: SpanClass) => {
if (finishedSpan.description && finishedSpan.description.indexOf(resourceName) !== -1) {
finishedSpan.startTimestamp = transactionSpan.startTimestamp + startTime - navigationOffset;
finishedSpan.timestamp = finishedSpan.startTimestamp + duration;
HazAT marked this conversation as resolved.
Show resolved Hide resolved
}
});
} else {
const resource = transactionSpan.child({
description: `${entry.initiatorType} ${resourceName}`,
op: `resource`,
});
resource.startTimestamp = transactionSpan.startTimestamp + startTime - navigationOffset;
resource.timestamp = resource.startTimestamp + duration;
// We remeber the entry script end time to calculate the difference to the first init mark
HazAT marked this conversation as resolved.
Show resolved Hide resolved
if (
entryScriptStartEndTime === undefined &&
entryScriptSrc &&
HazAT marked this conversation as resolved.
Show resolved Hide resolved
entryScriptSrc.indexOf(resourceName) > -1
) {
entryScriptStartEndTime = resource.timestamp;
}
addSpan(resource);
}
break;
default:
// Yo
HazAT marked this conversation as resolved.
Show resolved Hide resolved
}
});

if (entryScriptStartEndTime !== undefined && tracingInitMarkStartTime !== undefined) {
const evaluation = transactionSpan.child({
description: 'evaluation',
op: `script`,
});
evaluation.startTimestamp = entryScriptStartEndTime;
evaluation.timestamp = tracingInitMarkStartTime;
addSpan(evaluation);
}
if (global.performance) {
HazAT marked this conversation as resolved.
Show resolved Hide resolved
performance.clearMarks();
performance.clearMeasures();
performance.clearResourceTimings();
Tracing._performanceCursor = Math.max(performance.getEntries().length - 1, 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the intent of this _performanceCursor?

Copy link
Member

Choose a reason for hiding this comment

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

@rhcarvalho From what I understand, it's to skip over certain entries that were created after clearing the performance entries.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, since not all of the performance entries can be cleared with clear*() we need to remember which entries we already considered. And we call clear in the first place since browsers have an upper limit so this can overflow. TBH I am not sure if clearing this is a good practice but we'll see.

}

// tslint:enable: no-unsafe-any
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary here?!

}

/**
* Sets the status of the current active transaction (if there is one)
*/
Expand All @@ -352,6 +519,32 @@ export class Tracing implements Integration {
}
}

/**
* Adds offset to the span
*
* @param measureName name of the performance measure
* @param span Span to add data.offset to
*/
private static _addOffsetToSpan(measureName: string, span: SpanClass): void {
if (global.performance) {
const name = `#sentry-${measureName}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the # in front of sentry?

Copy link
Member

Choose a reason for hiding this comment

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

to make it "private" :D

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a global namespace (within the perf entries), #sentry-foo is no more private than sentry-foo, is it?

Copy link
Member

Choose a reason for hiding this comment

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

Nope. Likely nobody should use this prefix.

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'd like to use a "special char" in order if we ever need to do any string matching. No other reason than my personal style to prefix it with # ^^

performance.measure(name);
const measure = performance.getEntriesByName(name).pop();
Copy link
Contributor

Choose a reason for hiding this comment

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

There can be more than one entry with the same name!

In this very page here, open the developer console and run:

> performance.getEntriesByName("https://github.com/getsentry/sentry-javascript/pull/2474/review_comment/create")
(8) [PerformanceResourceTiming, PerformanceResourceTiming, PerformanceResourceTiming, PerformanceResourceTiming, PerformanceResourceTiming, PerformanceResourceTiming, PerformanceResourceTiming, PerformanceResourceTiming]

Copy link
Member Author

Choose a reason for hiding this comment

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

That's true, but since this mark /measure is only so short, it only lives to calculate the offset.
Therefore I think this can never return more than one.

if (measure) {
span.setData('offset', Tracing._msToSec(measure.duration));
}
performance.clearMeasures(name);
}
}

/**
* Converys ms time to s with ms precison
HazAT marked this conversation as resolved.
Show resolved Hide resolved
* @param time time in ms
*/
private static _msToSec(time: number): number {
return time / 1000;
}
Comment on lines +562 to +564
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd imagine we must already have this somewhere else? Though I prefer repeating it here than adding a dependency on a leftpad :P


/**
* Starts tracking for a specifc activity
*
Expand Down Expand Up @@ -382,9 +575,11 @@ export class Tracing implements Integration {
if (spanContext && _getCurrentHub) {
const hub = _getCurrentHub();
if (hub) {
const span = hub.startSpan(spanContext);
Tracing._addOffsetToSpan(`${name}${Tracing._currentIndex}`, span as SpanClass);
Tracing._activities[Tracing._currentIndex] = {
name,
span: hub.startSpan(spanContext),
span,
};
}
} else {
Expand Down Expand Up @@ -424,7 +619,7 @@ export class Tracing implements Integration {

if (activity) {
logger.log(`[Tracing] popActivity ${activity.name}#${id}`);
const span = activity.span;
const span = activity.span as SpanClass;
if (span) {
if (spanData) {
Object.keys(spanData).forEach((key: string) => {
Expand All @@ -438,6 +633,12 @@ export class Tracing implements Integration {
});
}
span.finish();
// If there is an offset in data, we need to shift timestamps towards it
if (span.data && typeof span.data.offset === 'number') {
span.startTimestamp += span.data.offset;
// tslint:disable-next-line: no-non-null-assertion
span.timestamp! += span.data.offset;
HazAT marked this conversation as resolved.
Show resolved Hide resolved
}
}
// tslint:disable-next-line: no-dynamic-delete
delete Tracing._activities[id];
Expand Down
2 changes: 1 addition & 1 deletion packages/apm/src/span.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ export class Span implements SpanInterface, SpanContext {
/**
* Timestamp when the span was created.
*/
public readonly startTimestamp: number = timestampWithMs();
public startTimestamp: number = timestampWithMs();

/**
* Internal start time tracked with a monotonic clock.
Expand Down