feat(rum): Add measurements support and web vitals #2909
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
size-limit report
|
markstory
reviewed
Sep 16, 2020
Co-authored-by: Mark Story <mark@sentry.io>
dashed
force-pushed
the
add-web-vitals
branch
from
1a2bafe
to
242a71d
Compare
7 tasks
dashed
force-pushed
the
add-web-vitals
branch
from
2cf4af0
to
29c4b52
Compare
rhcarvalho
approved these changes
Sep 22, 2020
There was a problem hiding this comment.
Thanks @dashed! The implementation looks clear. A few comments on things we could improve, merge at will ;)
Co-authored-by: Rodolfo Carvalho <rodolfo.carvalho@sentry.io>
Co-authored-by: Rodolfo Carvalho <rodolfo.carvalho@sentry.io>
Co-authored-by: Rodolfo Carvalho <rodolfo.carvalho@sentry.io>
Co-authored-by: Rodolfo Carvalho <rodolfo.carvalho@sentry.io>
dashed
requested review from
Zylphrex,
kamilogorek and
a team
and removed request for
kamilogorek
rhcarvalho
approved these changes
Sep 24, 2020
There was a problem hiding this comment.
LGTM. @dashed could you please add a README.md under .../web-vitals with a link to the repo and which commit it was vendored from?
Perhaps another level .../vendor/web-vitals and .../vendor/README.md could make it even more obvious?
@kamilogorek could you please have a quick look here, do you agree with this strategy?
|
@kamilogorek can you push these new revisions to beta? 😄 |
# Conflicts: # packages/tracing/src/browser/metrics.ts # packages/tracing/src/transaction.ts
|
@HazAT @rhcarvalho @kamilogorek Would this be good to merge? |
rhcarvalho
approved these changes
Oct 6, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Vendor
web-vitalsimplementation from: https://github.com/GoogleChrome/web-vitalsRemoved LCP implementation added in feat: Report LCP metric on pageload transactions #2624
Add support for setting
measurementsinterface for transaction events. This will not be officially documented as we're not allowing users to set their own measurements.Only
pageloadtransactions will have measurements attached to them.Intercept FP (first paint) and FCP (first contentful paint) and create measurements from their values.
Added
mark.lcp,mark.fcp,mark.fp, andmark.fid, to be timestamps for vertical line markers on the span view. See below screenshot for their usage:TODO
place measurements behind an option flag and disable it by default.EDIT: enabled by defaultweb-vitalslibrary