-
Notifications
You must be signed in to change notification settings - Fork 538
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
feat(instrumentation-document-load): add custom root span to document page load instrumentation #1092
Conversation
d8ccd65
to
f69a429
Compare
The diff is shorter with whitespace disabled: https://github.com/open-telemetry/opentelemetry-js-contrib/pull/1092/files?diff=split&w=1 |
@gregolsen, updating changelogs is actually done automatically during the release. Can someone knowledgeable in RUM look at this? @t2t2, @MSNev, @mhennoch |
6944d4a
to
9d2c339
Compare
Thank you @rauno56 - I got rid of the release update and rebased against the fresh main 🙇🏻 |
Will take a look tomorrow |
@mhennoch, have you had a moment? |
Looked OK but wanted to test it but haven't had the time. Maybe also @t2t2 can take a look. |
@t2t2, will yoy have a moment to check it out? |
Depends on if you want an answer to this PR wise or a big picture answer Like yes pr does what it says it does, if user wants to manually manage the document load span like this then yes this is the way But big picture wise, if I wanted a good set of rum instrumentations, I'd probably come up with a way to have documentLoad span be active from start of instrumentations being loaded until page load event happens. This would probably also decouple html resources (img, link, script tags) from document load instrumentation somewhat allowing it to also work for SPA use, making both documentLoad and userInteraction work similarly: As noted on image this sort of idea wouldn't work for those who would base interaction duration from root span duration (but rather would need post-processing once sure that no more data comes from the trace). For those, quoting PR "Ideally, single-page app should be able to attach more spans to the root span and have control over when the root span is ended.", would be better. So the more automatically possible behavior would probably be better as an alternative choice of instrumentation for those who can use it |
A bit more context about our setup that may help here: |
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
This PR was closed because it has been stale for 14 days with no activity. |
Which problem is this PR solving?
Page load instrumentation creates and ends its own root span. However, for a typical single-page JS app, transaction is not completed after the initial page load is finished. Ideally, single-page app should be able to attach more spans to the root span and have control over when the root span is ended.
Short description of the changes
This PR extends instrumentation with an optional
rootSpan
config parameter. WhenrootSpan
is passed instrumentation attaches all the child spans/attributes to thatrootSpan
and doesn't end the span.Checklist
npm run test-all-versions
for the edited package(s) on the latest commit if applicable.