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

feat(apm): Update Span timing from absolute ref #2479

Merged
merged 1 commit into from
Mar 9, 2020
Merged

feat(apm): Update Span timing from absolute ref #2479

merged 1 commit into from
Mar 9, 2020

Conversation

rhcarvalho
Copy link
Contributor

@rhcarvalho rhcarvalho commented Mar 9, 2020 •
edited
Loading

Use performance.timeOrigin as the absolute reference to update Span start and end timestamps.

With this in, we could remove the data.offset value from Spans, and instead/additionally use something else to mark modifications to spans that might make it easier to search/filter.

@rhcarvalho rhcarvalho requested a review from HazAT March 9, 2020 12:39
@getsentry-bot
Copy link
Contributor

getsentry-bot commented Mar 9, 2020 •
edited
Loading

Warnings
⚠️ Please add a changelog entry for your changes.
Messages
📖

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

Generated by 🚫 dangerJS against 3a5d9a1

@rhcarvalho
Copy link
Contributor Author

  • ERROR: /home/travis/build/getsentry/sentry-javascript/packages/apm/src/integrations/tracing.ts[388, 58]: timing is deprecated.

https://developer.mozilla.org/en-US/docs/Web/API/PerformanceTiming/navigationStart
https://developer.mozilla.org/en-US/docs/Web/API/Performance/timeOrigin

performance.timing.navigationStart is deprecated, though more widely supported than performance.timeOrigin. In particular, performance.timeOrigin is undefined in Safari.

In Chrome Linux 80.0.3987.122 (Official Build) (64-bit), performance.timeOrigin is more precise than performance.timing.navigationStart:

> performance.timing.navigationStart
1583759502588
> performance.timeOrigin
1583759502588.7002

Node.js also has timeOrigin in https://nodejs.org/api/perf_hooks.html#perf_hooks_performance_timeorigin.

Going to update to use timeOrigin with a fallback to navigationStart.

@rhcarvalho rhcarvalho force-pushed the rhcarvalho/absolute-perf-timing branch from 89db11c to 830362c Compare March 9, 2020 14:47
HazAT
HazAT approved these changes Mar 9, 2020
View reviewed changes
Copy link
Member

@HazAT HazAT left a comment •
edited
Loading

Choose a reason for hiding this comment

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

👌 (after linter fixes)

@rhcarvalho rhcarvalho marked this pull request as ready for review March 9, 2020 14:57
@rhcarvalho rhcarvalho force-pushed the rhcarvalho/absolute-perf-timing branch from 830362c to 3094034 Compare March 9, 2020 15:14
@rhcarvalho
feat(apm): Update Span timing from absolute ref
3a5d9a1 
Use performance.timeOrigin as the absolute reference to update Span
start and end timestamps.
@rhcarvalho rhcarvalho force-pushed the rhcarvalho/absolute-perf-timing branch from 3094034 to 3a5d9a1 Compare March 9, 2020 15:15
dashed
dashed reviewed Mar 9, 2020
View reviewed changes
packages/apm/src/integrations/tracing.ts
navigationOffset = transactionSpan.data.offset;
}

const timeOrigin = Tracing._msToSec(performance.timeOrigin);
Copy link
Member

Choose a reason for hiding this comment

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

@rhcarvalho I came across this polyfill to replace an older Chrome API: https://developers.google.com/web/updates/2017/12/chrome-loadtimes-deprecated#startloadtime

wondering if we could make use of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

Let's open the can of worms.
Need to make sure whether the times in entries are relative to timeOrigin or navigationStart, if the two ever differ -- @HazAT and I have only seen performance.getEntriesByType('navigation')[0].startTime === 0.

Copy link
Member

Choose a reason for hiding this comment

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

This potential update could be something that can be in a new PR; not in this PR 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://developer.mozilla.org/en-US/docs/Web/API/PerformanceEntry/startTime

The startTime property returns the first recorded timestamp of the performance entry.

The value returned by this property depends on the performance entry's type:
...
"navigation" - returns the timestamp with a value of "0".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

w3c/navigation-timing#43

Seems that the timeOrigin and navigationStart should be the same in practice.

In that case, startloadtime is unnecessarily verbose, giving us nothing more than what we have now? @dashed wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

@rhcarvalho that makes sense. I didn't realize navigation is essentially 0. Thank you for investigating this further 👍

dashed
dashed approved these changes Mar 9, 2020
View reviewed changes
Copy link
Member

@dashed dashed left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@rhcarvalho rhcarvalho merged commit 76da9e7 into master Mar 9, 2020
@rhcarvalho rhcarvalho deleted the rhcarvalho/absolute-perf-timing branch March 9, 2020 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dashed dashed dashed approved these changes

@HazAT HazAT HazAT approved these changes

@kamilogorek kamilogorek Awaiting requested review from kamilogorek

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rhcarvalho @getsentry-bot @dashed @HazAT