fix: Provide better descriptions for the performance navigation timing spans #3245
Conversation
size-limit report
|
| addPerformanceNavigationTiming({ transaction, entry, event: 'loadEvent', timeOrigin }); | ||
| addPerformanceNavigationTiming({ transaction, entry, event: 'connect', timeOrigin }); | ||
| addPerformanceNavigationTiming({ transaction, entry, event: 'secureConnection', timeOrigin, eventEnd: 'connectEnd', description: 'secureConnection (TLS)' }); | ||
| addPerformanceNavigationTiming({ transaction, entry, event: 'fetch', timeOrigin, eventEnd: 'domainLookupStart', description: 'cache' }); |
There was a problem hiding this comment.
Is this guaranteed to be coming from the browser cache?
There was a problem hiding this comment.
"AppCache" is a deprecated browser feature:
- https://developer.mozilla.org/en-US/docs/Web/HTML/Using_the_application_cache
- https://web.dev/appcache-removal/
But instead of referring it to "AppCache", I just used "cache" as a description. It seems that there are other cache mechanisms being used. See: w3c/resource-timing#256
| addPerformanceNavigationTiming({ transaction, entry, event: 'connect', timeOrigin }); | ||
| addPerformanceNavigationTiming({ transaction, entry, event: 'secureConnection', timeOrigin, eventEnd: 'connectEnd', description: 'secureConnection (TLS)' }); | ||
| addPerformanceNavigationTiming({ transaction, entry, event: 'fetch', timeOrigin, eventEnd: 'domainLookupStart', description: 'cache' }); | ||
| addPerformanceNavigationTiming({ transaction, entry, event: 'domainLookup', timeOrigin }); |
There was a problem hiding this comment.
As per PR description, isn't this missing a description: 'DNS'?
| addPerformanceNavigationTiming({ transaction, entry, event: 'domContentLoadedEvent', timeOrigin }); | ||
| addPerformanceNavigationTiming({ transaction, entry, event: 'loadEvent', timeOrigin }); | ||
| addPerformanceNavigationTiming({ transaction, entry, event: 'connect', timeOrigin }); | ||
| addPerformanceNavigationTiming({ transaction, entry, event: 'secureConnection', timeOrigin, eventEnd: 'connectEnd', description: 'secureConnection (TLS)' }); |
There was a problem hiding this comment.
@dashed do you have a link to where this one is spec'ed? secureConnection (TLS) looks kinda verbose compared to domainLookup -> DNS
Couldn't we say TLS/SSL? or TLS?
| eventEnd?: string; | ||
| description?: string; | ||
| }): void { | ||
| const { transaction, entry, event, timeOrigin, eventEnd, description } = props; |
There was a problem hiding this comment.
Does it pay-off moving all args into a single object just to deconstruct it here?!
There was a problem hiding this comment.
Oh well, one advantage is being able to name an argument and omit others. 👍
dashed
force-pushed
the
navigation-timing-descriptions
branch
from
94a7d03 to
3176e64
Compare
|
@rhcarvalho I've updated the PR description with the screenshot of the description changes. |
There was a problem hiding this comment.
I'm not sure I understand the rationale behind this change. We don't have "processing" or "load" spans from your picture, so it's not like we're going totally consistent with that. The only current description I think is a little confusing is "fetch" meaning AppCache, but that's deprecated anyways, so should we be adding this stuff just for that?
|
@alxu-sentry this change is to make the descriptions of the spans we already instrument better. The processing and load spans could be added later in the future. |
mneh, i'm just a little skeptical that the new messages are actually an improvement though. in my personal opinion as a user "domainLookup" is more clear because it must be coming from the performance timing api. it doesn't matter that much though |
There was a problem hiding this comment.
Sorry for the huge delay, this got lost among other notifications.
Thanks @dashed!
dashed
force-pushed
the
navigation-timing-descriptions
branch
from
3176e64 to
e67b44a
Compare
Screenshot provided by Rodolfo B.:
Today performance navigation timing spans are created with descriptions that do not match the following diagram (specifically in regions outlined in magenta):
The descriptions for these spans use the event name which may not be sufficient. Thus, we rename some as follows:
What it looks like: