-
Notifications
You must be signed in to change notification settings - Fork 518
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(longtasks): allow callback to add span attributes on collection #863
feat(longtasks): allow callback to add span attributes on collection #863
Conversation
|
a0131ed
to
0244cc2
Compare
plugins/web/opentelemetry-instrumentation-long-task/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/web/opentelemetry-instrumentation-long-task/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/web/opentelemetry-instrumentation-long-task/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/web/opentelemetry-instrumentation-long-task/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/web/opentelemetry-instrumentation-long-task/src/instrumentation.ts
Outdated
Show resolved
Hide resolved
plugins/web/opentelemetry-instrumentation-long-task/src/types.ts
Outdated
Show resolved
Hide resolved
plugins/web/opentelemetry-instrumentation-long-task/src/types.ts
Outdated
Show resolved
Hide resolved
plugins/web/opentelemetry-instrumentation-long-task/test/longTask.test.ts
Outdated
Show resolved
Hide resolved
plugins/web/opentelemetry-instrumentation-long-task/src/types.ts
Outdated
Show resolved
Hide resolved
Thanks @crellison for addressing everything 💯 |
This adds a callback parameter to the LongTaskInstrumentation to be executed when a longtask is observed. The return value of the callback is an object of span attributes to be attached to the longtask span. This feature allows sites using client-side navigation to add window location details to spans and enables hydrating spans with relevant information on application state.
15fe93b
to
1fa561e
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.
LGTM.
Thanks for the contribution :)
I'll be happy for another review from someone with stronger web instrumentation background
@open-telemetry/javascript-approvers
1fa561e
to
bd17f31
Compare
@blumamir Do you have a way of checking in with any of the JS approvers to see if someone is available to review? |
plugins/web/opentelemetry-instrumentation-long-task/test/longTask.test.ts
Outdated
Show resolved
Hide resolved
@t2t2, the component owner, has approved, I think we are good to go, unless @crellison wants to address the nit above. |
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.
LGTM
Probably @MSNev at this point. @vmarchaud has more web experience than me. Bart was the person who did most of our web reviews before. |
Which problem is this PR solving?
longtask
spansShort description of the changes
This adds a callback parameter to the LongTaskInstrumentation to be executed when a longtask is observed. The return value of the callback is an object of span attributes to be attached to the longtask span.
This feature is important for web applications, as additional details on application state may be relevant to long tasks and can help in correlating long tasks with slow code. This feature allows sites using client-side navigation to add window location details to spans and enables hydrating spans with relevant information from application state.
Checklist
npm run test-all-versions
for the edited package(s) on the latest commit if applicable. (not applicable, but checking it rather than removing)