-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(browser): Add option to explicitly end pageload span via reportPageLoaded()
#17697
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
Conversation
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.
|
* A hook for the browser tracing integrations to trigger the end of a page load span. | ||
* @returns {() => void} A function that, when executed, removes the registered callback. | ||
*/ | ||
public on(hook: 'endPageloadSpan', callback: () => void): () => void; |
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 decided to go with the client hooks approach instead of exposing a method on browserTracingIntegration
because in contrast to all other integrations, we don't hide the specific implementation of browserTracingIntegration
behind defineIntegration
. So adding a method would indeed be public API (we also can't introduce defineIntegration
now as that would be a breaking change).
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.
IMHO this is a nice enough API for this anyhow, I like it!
3623454
to
c1762f1
Compare
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.
name nit maybe, but also no strong opinion either way - overall this is really nice, love to finally see this!
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 like this :)
Was thinking about renaming the option to enableManualPageLoadEnd
or something different including the word manual
but enableReportPageLoaded
is actually nice because it resembles the actual API you need to call.
That's great!! I'm just looking for this feature. Wonder when this will release. |
It was released in 10.13.0 :) |
This PR adds new functionality to
browserTracingIntegration
: By setting the newenableReportPageLoaded
option totrue
, users can take full control* of the pageload span duration. It will stay active untilSentry.reportPageLoaded()
is called*. This new functionality is opt-in and nothing changes for the default idle span mechanism.* there are a couple of caveats and implications to this behaviour:
finalTimeout
still acts as a safeguard to end the pageload span if the final timeout (30s by default) is reached. Just like by default, the span will be ended after the final timeout and the ending reason is set to'finalTimeout'
.enableReportPageLoaded: true
. In this case, the pageload span's end time stamp or duration will not be adjusted.'cancelled'
. My reasoning for this behaviour is that the entire SDK behaviour relies on the navigation span being another root span (with the exception of redirects). Ending the pageload span manually is already quite hard, so I'd argue we leave it at this for now and revisit potentially on request.Usage
Reviewers, happy to chat about the decisions and also happy to hear better suggestions!
Out of scope: Navigation spans
With this PR we don't provide users an API to control navigation span ending in the same way and I'd suggest we leave it this way for the time being. The reason is that a pageload is started once (at SDK init time) automatically and therefore, users only need to control the end time of this one span. Which IMHO is already hard enough to get right.
If we throw navigation spans into the mix we're in a bit of a weird state where the SDK takes care of automatically starting spans repeatedly, but hands down the responsibility to end them to users. I would argue that for users interested in taking control over the navigation span, too, the more fitting use case is to also start them like so:
The missing piece here is an option to control idling behavior but we can adjust this on demand and perhaps pass down the newly introduced
trimIdleSpanEndTimestamp
tostartBrowserTracingNavigationSpan
.closes #14810