feat(instrumentation-document-load): performance paint timing events#484
feat(instrumentation-document-load): performance paint timing events#484obecny merged 7 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## main #484 +/- ##
=======================================
Coverage 94.81% 94.81%
=======================================
Files 150 152 +2
Lines 9153 9169 +16
Branches 904 909 +5
=======================================
+ Hits 8678 8694 +16
Misses 475 475
|
johnbley
left a comment
johnbley
left a comment
There was a problem hiding this comment.
Generally looks good, but I think the event names can be improved.
| }; | ||
|
|
||
| const performancePaintNames = { | ||
| 'first-paint': EventNames.FIRST_PAINT, |
There was a problem hiding this comment.
I don't see a lot of dashes in names/semantic conventions in otel... . and _ yes, but not -. Perhaps the camel case names are fine as they are? Most other browser performance event names are passed through unchanged.
There was a problem hiding this comment.
So API defines them as first-paint and I proposed to use firstPaint.
The EventNames enum is for our OT enum names and here I define how to map them.
Other events are camel cased and I wanted to follow this convention.
There was a problem hiding this comment.
Oh, I was reading it backwards! Apologies!
There was a problem hiding this comment.
@johnbley does it mean you are ok with this changes then ?
There was a problem hiding this comment.
Yes, it looks good. Sorry I didn't officially, approve, will do so.
|
@kkruk-sumo can you please fix the tests, and perhaps allow maintainers to allow changes so this way it will be easier to merge when it is outdated |
|
@obecny Sorry. Firstly I had issues with eslint (finally found out that I had the wrong eslint version) and then |
|
LGTM |
|
@obecny Can we merge this PR? |
|
@kkruk-sumo yes, but it cannot be merged until it is rebased with base branch . So please merge with latest or allow editing by maintainers :) thx |
|
@obecny Apologies. I didn't know the PR needs to be up to date. Unfortunately I don't have permissions to allow maintainers to edit this PR or at least I don't see such option. |
merged :), thx for contribution |
4 participants
Which problem is this PR solving?
Added new events to the root span
documentLoad:firstPaintfirstContentfulPaintThese events can be used in RUM and to better understand website's loading performance.
Short description of the changes
The
_getEntriesmethod has been moved to utils as it doesn't need to have class instance. New utiladdSpanPerformancePaintEventsenhances given span with the collected paint events.