-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(browser): Fix ElementTiming span timestamps and attribute names #19261
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
Closed
+202
−215
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: I checked the
startTimeproperty and I think I'm getting confused on how the span duration is being determined, it seems like it represents the opposite of what we are doing here for the start time?I think what you have here make sense, but was wondering if it is consistently correct.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand the definition of
startTimecorrectly, the prop always returns the "longest" time available ( i.e.renderTime if !== 0, otherwiseloadTime. My idea for using the shorter of the two is that we can show a timespan for images representing the relative rendering time. Does this make sense? Happy to switch tostartTime, but unless I'm missing something, we'd always generate point-in-time spans. WDYT?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never been so confused by a spec lol, okay so here is what I understand:
For text: we don't know the
startTimeof the span, we know the render time tho. So in this caserelativeStartTime = renderTimeandrelativeEndTime = renderTime. Which makes them equal.For images: Each value tells a different story because they aren't related, load time is the timestamp it took to be loaded and attached to the element, while rendering should happen after. So kinda each represent an "endTime" of two different spans, a load span and a render span, in case of text we don't have a "load" span.
The question here is if
renderTimeandloadTimeare both end times for each respective span, what's the start time? I don't think we have that information here. Which makes me think point-in-time spans here make sense for these cases, or a metric even.There could be a long winded way to guess the start time with resource timing and try to match the resource with the image element but ehhh, I don't know if it is worth it.
Does that make any sense or did I confuse myself 😂 I will approve anyways to not drag this any longer, but just wanted to see what you think first.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, correct, text element timing will always be duration 0 (for better or worse)
Hmm I just assumed that rendering would happen right after loading completed, so my thinking was, instead of having yet again no duration at all for the span, we use the relative rendering time (i.e. renderTime - loadTime) for the span duration. But maybe that assumption is wrong?
So you mean we should create two spans? We could do that (tho both would have duration 0 then) but it would increase quota usage/billing accordingly.
I think theoretically, it should be
performance.timeOrigin, right? but if we let the span start from there, it would mess up the trace waterfall if an elementTiming span was added to e.g. a navigation span, rather than the pageload. I think the root issue is that ElementTiming, like classic web vitals, just doesn't play well withSPAs :(
Yeah, the SPA limitation basically was my reasoning for rather having point-in-time spans than potentially super long ones.
Interesting idea! We could give it a try though I believe the timing would be weird again, simply because ElementTiming measures its values from
performance.timeOrigin. So if we find out when we actually started loading the image via the ResourceTiming, and let the span start from this time on, the semantics around the span duration would somehow not tell us much either (?). Also, assuming we get aResourceTimingentry, our SDK should be able to collect aresource.imgspan anyway.Hmm that's a good point! Radical idea: What if we just delete all of the element timing span logic and create a
browserMetricsIntegration(or a more specific one)? Tracking Element timing spans wasn't documented, it never worked and the only thing we'd need to leave is theenableElementTimingoption.Though, before we do this, we need to think which metrics we'd actually collect.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in favor of this actually, for one we start breaking up the tracing integration, secondly, does that mean we could re-work into a metric instead? Happy to take that on if you have too much on your plate.
I think you are right on the other points, the span timing no matter where we assign its start and end doesn't make full sense, and then matching the same resource in two event pipelines is just asking for non-ending spans or spans with no start time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@logaretm if you could take a look at this and think about how we could track ET as metrics instead of spans that would be greatly appreciated! I'll hold off from merging this for the moment.