feat[apm]: Use Performance API #2474

HazAT · 2020-03-06T12:08:54Z

If available we use https://developer.mozilla.org/en-US/docs/Web/API/Performance API to get more insight into page load. This is especially helpful for the initial pageLoad since we get a lot of additional info there.

In addition to new spans, we also use the timings of the performance API to update our measurements.

Before:

After:

Internal note:

We need to document that if users mark their entry script with data-entry="true" like:
<script src="..." data-entry="true"></script>
We are able to measure the script evaluation.

ref: https://github.com/getsentry/sentry/pull/17501/files#diff-cd6de428a381ccfe274524ae3f0e9ea1L41

getsentry-bot · 2020-03-06T12:14:29Z

	Messages
📖	@sentry/browser bundle gzip'ed minified size: (ES5: 16.7412 kB) (ES6: 15.7559 kB)
📖	✅ TSLint passed

Generated by 🚫 dangerJS against 3b34841

rhcarvalho

Kamil recently added some fallback code to support Node 6 and whatever else don't have the performance API, we may want to integrate with that -- even if it means explicitly disabling some functionality depending on the availability of the API.

HazAT · 2020-03-06T13:17:31Z

This integration only works in browsers so it's not relevant I think.

dashed · 2020-03-06T13:24:20Z

packages/apm/src/integrations/tracing.ts

+        // 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') {


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

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.

rhcarvalho

Let's unit test the span update logic to avoid breaking it when we touch this code in the future?

rhcarvalho · 2020-03-06T12:57:30Z

packages/apm/src/integrations/tracing.ts

+      performance.clearMarks();
+      performance.clearMeasures();
+      performance.clearResourceTimings();
+      Tracing._performanceCursor = Math.max(performance.getEntries().length - 1, 0);


What's the intent of this _performanceCursor?

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

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.

packages/apm/src/integrations/tracing.ts

rhcarvalho · 2020-03-06T13:13:29Z

packages/apm/src/integrations/tracing.ts

+      Tracing._performanceCursor = Math.max(performance.getEntries().length - 1, 0);
+    }
+
+    // tslint:enable: no-unsafe-any


Is this necessary here?!

rhcarvalho · 2020-03-06T13:14:26Z

packages/apm/src/integrations/tracing.ts

+   */
+  private static _addOffsetToSpan(measureName: string, span: SpanClass): void {
+    if (global.performance) {
+      const name = `#sentry-${measureName}`;


Why the # in front of sentry?

to make it "private" :D

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

Nope. Likely nobody should use this prefix.

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 # ^^

rhcarvalho · 2020-03-06T13:21:24Z

packages/apm/src/integrations/tracing.ts

+    if (global.performance) {
+      const name = `#sentry-${measureName}`;
+      performance.measure(name);
+      const measure = performance.getEntriesByName(name).pop();


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]

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.

packages/apm/src/integrations/tracing.ts

rhcarvalho · 2020-03-06T13:23:37Z

packages/apm/src/integrations/tracing.ts

+  private static _msToSec(time: number): number {
+    return time / 1000;
+  }


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

dashed · 2020-03-07T08:49:30Z

@rhcarvalho It looks like the code paths with these changes will only be run in browsers. For node.js, there are specialized performance instrumentation: https://nodejs.org/api/perf_hooks.html#perf_hooks_performanceentry_entrytype

I imagine that could be supported later on.

kamilogorek · 2020-03-09T08:52:00Z

packages/apm/src/integrations/tracing.ts

+   * 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


revolver ocelot

— revolver ocelot

packages/apm/src/integrations/tracing.ts

kamilogorek · 2020-03-09T08:54:54Z

packages/apm/src/integrations/tracing.ts

+    ) {
+      navigationOffset = transactionSpan.data.offset;
+    }
+    // tslint:disable-next-line: completed-docs


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

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

kamilogorek · 2020-03-09T08:55:53Z

packages/apm/src/integrations/tracing.ts

+        op: 'browser',
+      });
+      // tslint:disable: no-unsafe-any
+      span.startTimestamp = parent.startTimestamp + Tracing._msToSec(entry[`${event}Start`] as number);


It can be written as

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

Nope,

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

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

packages/apm/src/integrations/tracing.ts

kamilogorek

.

Co-Authored-By: Rodolfo Carvalho <[email protected]>

…ascript into apm-performance-api

HazAT added 3 commits March 6, 2020 09:26

feat: Use performance api

4d5944d

feat: Update ops & descriptions

8f28d75

feat: Handle navigation changes correctly

861bb37

HazAT requested a review from kamilogorek as a code owner March 6, 2020 12:08

HazAT self-assigned this Mar 6, 2020

chore: Code comments

e29e6ff

rhcarvalho reviewed Mar 6, 2020

View reviewed changes

dashed reviewed Mar 6, 2020

View reviewed changes

rhcarvalho reviewed Mar 6, 2020

View reviewed changes

HazAT mentioned this pull request Mar 6, 2020

feat: Add data entry for APM getsentry/sentry#17501

Closed

1 task

kamilogorek reviewed Mar 9, 2020

View reviewed changes

packages/apm/src/integrations/tracing.ts Show resolved Hide resolved

kamilogorek reviewed Mar 9, 2020

View reviewed changes

packages/apm/src/integrations/tracing.ts Outdated Show resolved Hide resolved

kamilogorek reviewed Mar 9, 2020

View reviewed changes

packages/apm/src/integrations/tracing.ts Outdated Show resolved Hide resolved

kamilogorek reviewed Mar 9, 2020

View reviewed changes

packages/apm/src/integrations/tracing.ts Outdated Show resolved Hide resolved

kamilogorek reviewed Mar 9, 2020

View reviewed changes

packages/apm/src/integrations/tracing.ts Outdated Show resolved Hide resolved

kamilogorek suggested changes Mar 9, 2020

View reviewed changes

HazAT and others added 3 commits March 9, 2020 11:01

Apply suggestions from code review

789ee31

Co-Authored-By: Rodolfo Carvalho <[email protected]>

chore: CodeReview

eb55285

Merge branch 'master' into apm-performance-api

3b34841

kamilogorek approved these changes Mar 9, 2020

View reviewed changes

HazAT added 2 commits March 9, 2020 11:40

ref: Check for performance API

4f78e54

Merge branch 'apm-performance-api' of github.com:getsentry/sentry-jav…

e395920

…ascript into apm-performance-api

HazAT merged commit 0bc7a4d into master Mar 9, 2020

HazAT deleted the apm-performance-api branch March 9, 2020 10:45

rhcarvalho mentioned this pull request Mar 9, 2020

ref(apm): Simplify Tracing._addOffsetToSpan #2477

Merged

snyk-bot mentioned this pull request Apr 10, 2020

[Snyk] Upgrade @sentry/node from 5.14.0 to 5.14.2 guerronc/ecommerce-api#5

Open

This was referenced Apr 22, 2020

[Snyk] Upgrade @sentry/node from 5.13.2 to 5.15.4 tonesto7/homebridge-smartthings-v2#132

Closed

[Snyk] Upgrade @sentry/node from 5.12.5 to 5.15.4 liquality/atomicagent#50

Closed

[Snyk] Upgrade @sentry/node from 5.5.0 to 5.15.4 AtomicLoans/liquidator#26

Open

snyk-bot mentioned this pull request May 5, 2020

[Snyk] Upgrade @sentry/node from 5.9.0 to 5.15.4 alphagov/pay-toolbox#376

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat[apm]: Use Performance API #2474

feat[apm]: Use Performance API #2474

HazAT commented Mar 6, 2020 •

edited

Loading

getsentry-bot commented Mar 6, 2020 •

edited

Loading

rhcarvalho left a comment

HazAT commented Mar 6, 2020

dashed Mar 6, 2020

HazAT Mar 6, 2020 •

edited

Loading

rhcarvalho left a comment

rhcarvalho Mar 6, 2020

dashed Mar 6, 2020

HazAT Mar 9, 2020

rhcarvalho Mar 6, 2020

rhcarvalho Mar 6, 2020

dashed Mar 6, 2020

rhcarvalho Mar 6, 2020

dashed Mar 6, 2020

HazAT Mar 9, 2020

rhcarvalho Mar 6, 2020

HazAT Mar 9, 2020

rhcarvalho Mar 6, 2020

dashed commented Mar 7, 2020

kamilogorek Mar 9, 2020 •

edited

Loading

kamilogorek Mar 9, 2020

HazAT Mar 9, 2020

kamilogorek Mar 9, 2020 •

edited

Loading

HazAT Mar 9, 2020 •

edited

Loading

kamilogorek left a comment

feat[apm]: Use Performance API #2474

feat[apm]: Use Performance API #2474

Conversation

HazAT commented Mar 6, 2020 • edited Loading

Internal note:

getsentry-bot commented Mar 6, 2020 • edited Loading

rhcarvalho left a comment

Choose a reason for hiding this comment

HazAT commented Mar 6, 2020

Choose a reason for hiding this comment

HazAT Mar 6, 2020 • edited Loading

Choose a reason for hiding this comment

rhcarvalho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dashed commented Mar 7, 2020

kamilogorek Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamilogorek Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

HazAT Mar 9, 2020 • edited Loading

Choose a reason for hiding this comment

kamilogorek left a comment

Choose a reason for hiding this comment

HazAT commented Mar 6, 2020 •

edited

Loading

getsentry-bot commented Mar 6, 2020 •

edited

Loading

HazAT Mar 6, 2020 •

edited

Loading

kamilogorek Mar 9, 2020 •

edited

Loading

kamilogorek Mar 9, 2020 •

edited

Loading

HazAT Mar 9, 2020 •

edited

Loading