Skip to content
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

Merged
merged 12 commits into from
Feb 16, 2022

Conversation

crellison
Copy link
Contributor

Which problem is this PR solving?

  • ability to add application-specific context as attributes to longtask spans

Short 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

  • Ran npm run test-all-versions for the edited package(s) on the latest commit if applicable. (not applicable, but checking it rather than removing)

@crellison crellison requested a review from a team January 31, 2022 21:46
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 31, 2022

CLA Signed

The committers are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Jan 31, 2022

Codecov Report

Merging #863 (2f1ddd8) into main (b9a1381) will increase coverage by 0.61%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #863      +/-   ##
==========================================
+ Coverage   95.29%   95.91%   +0.61%     
==========================================
  Files          10       13       +3     
  Lines         701      856     +155     
  Branches      142      178      +36     
==========================================
+ Hits          668      821     +153     
- Misses         33       35       +2     
Impacted Files Coverage Δ
...y-instrumentation-long-task/src/instrumentation.ts 94.73% <100.00%> (ø)
...ation-user-interaction/src/enums/AttributeNames.ts
...y-instrumentation-long-task/src/instrumentation.ts
...umentation-user-interaction/src/instrumentation.ts
...strumentation-document-load/src/instrumentation.ts
...trumentation-document-load/src/enums/EventNames.ts
...entation-document-load/src/enums/AttributeNames.ts
...lemetry-instrumentation-document-load/src/utils.ts
...strumentation-document-load/src/instrumentation.ts 98.79% <0.00%> (ø)
...entation-document-load/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)
... and 8 more

@crellison crellison force-pushed the crellison/long-task-add-context branch 2 times, most recently from a0131ed to 0244cc2 Compare January 31, 2022 22:00
@blumamir
Copy link
Member

blumamir commented Feb 6, 2022

Thanks @crellison for addressing everything 💯
This is great work.
Added last minor comment and then it can be merged as far as I am concerned

@vmarchaud vmarchaud added the enhancement New feature or request label Feb 6, 2022
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.
@crellison crellison force-pushed the crellison/long-task-add-context branch from 15fe93b to 1fa561e Compare February 6, 2022 23:19
Copy link
Member

@blumamir blumamir left a 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

@crellison crellison force-pushed the crellison/long-task-add-context branch from 1fa561e to bd17f31 Compare February 7, 2022 14:53
@blumamir blumamir requested a review from a team February 8, 2022 07:00
@crellison
Copy link
Contributor Author

@blumamir Do you have a way of checking in with any of the JS approvers to see if someone is available to review?

@blumamir
Copy link
Member

blumamir commented Feb 14, 2022

@blumamir Do you have a way of checking in with any of the JS approvers to see if someone is available to review?

I suggest writing in the otel-js CNCF slack channel.
@dyladan - Who from the approvers is most familiar with the web instrumentations?

@rauno56
Copy link
Member

rauno56 commented Feb 14, 2022

@t2t2, the component owner, has approved, I think we are good to go, unless @crellison wants to address the nit above.

Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dyladan
Copy link
Member

dyladan commented Feb 16, 2022

@dyladan - Who from the approvers is most familiar with the web instrumentations?

Probably @MSNev at this point. @vmarchaud has more web experience than me. Bart was the person who did most of our web reviews before.

@dyladan dyladan merged commit 1f68004 into open-telemetry:main Feb 16, 2022
@crellison crellison deleted the crellison/long-task-add-context branch February 17, 2022 06:08
This was referenced Feb 25, 2022
@chingor13 chingor13 mentioned this pull request Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants