fix(browser): Fix ElementTiming span timestamps and attribute names#19261
fix(browser): Fix ElementTiming span timestamps and attribute names#19261
Conversation
Lms24
changed the title
Codecov Results 📊Generated by Codecov Action |
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
|
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you apply the label |
|
Closing due to inactivity after stale warning. Comment or reopen when ready to continue, and use |
|
argh damnit I need to revisit this. Reopening |
Lms24
force-pushed
the
lms/fix-browser-elementtiming-timestamps
branch
from
560fe36 to
eec5f5e
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Lms24
force-pushed
the
lms/fix-browser-elementtiming-timestamps
branch
from
eec5f5e to
66473da
Compare
Lms24
force-pushed
the
lms/fix-browser-elementtiming-timestamps
branch
from
66473da to
362bd03
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Lms24
requested review from
a team,
logaretm and
nicohrubec
and removed request for
a team
| const relativeStartTime = loadTime > 0 ? loadTime : renderTime; | ||
| const relativeEndTime = renderTime > 0 ? renderTime : loadTime; |
There was a problem hiding this comment.
Q: I checked the startTime property 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.
There was a problem hiding this comment.
If I understand the definition of startTime correctly, the prop always returns the "longest" time available ( i.e. renderTime if !== 0, otherwise loadTime. 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 to startTime, but unless I'm missing something, we'd always generate point-in-time spans. WDYT?
There was a problem hiding this comment.
Never been so confused by a spec lol, okay so here is what I understand:
For text: we don't know the startTime of the span, we know the render time tho. So in this case relativeStartTime = renderTime and relativeEndTime = 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 renderTime and loadTime are 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.
There was a problem hiding this comment.
For text: ... Which makes them equal.
Yes, correct, text element timing will always be duration 0 (for better or worse)
For images: ... Each value tells a different story because they aren't related
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 kinda each represent an "endTime" of two different spans
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.
The question here is if renderTime and loadTime are both end times for each respective span, what's the start time?
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 with
SPAs :(
Which makes me think point-in-time spans here make sense for these cases
Yeah, the SPA limitation basically was my reasoning for rather having point-in-time spans than potentially super long ones.
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.
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 a ResourceTiming entry, our SDK should be able to collect a resource.img span anyway.
or a metric even.
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 the enableElementTiming option.
Though, before we do this, we need to think which metrics we'd actually collect.
There was a problem hiding this comment.
Radical idea: What if we just delete all of the element timing span logic and create a browserMetricsIntegration
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.
@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.
logaretm
left a comment
logaretm
left a comment
There was a problem hiding this comment.
Unblocking this, but we have an alternate path.
|
I will close this in favour of #19869. This approach has proven to be the wrong one and I think ET is an ideal first testbed for emitting actual browser metrics. |
2 participants
This PR fixes a bug where previously, element timing spans were discarded due to invalid relative timestamps.
Given this rendered element timing span collection unfunctnional, I also took the opportunity to slightly rework span start and end semantics as well as attribute names (getsentry/sentry-conventions#284).
We now always start spans at the first element timing value and stop them at the last. Effectively, this means that only image-paint spans will have a duration (the actual render time), while text-paint spans will have no duration. The problem is, if we start the span at
performance.timeOrigin(i.e. the beginning of the initial pageload), collecting element timing spans makes no sense anymore after the initial pageload span tree. One can argue about the usefulness the ElementTiming API in SPAs in general though. Though this means the only alternative would be to completely stop recording ElementTiming spans after the initial pageload.closes #19260