-
Notifications
You must be signed in to change notification settings - Fork 534
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: Long Tasks instrumentation #757
Conversation
Codecov Report
@@ Coverage Diff @@
## main #757 +/- ##
==========================================
- Coverage 96.87% 96.26% -0.62%
==========================================
Files 11 13 +2
Lines 641 697 +56
Branches 127 137 +10
==========================================
+ Hits 621 671 +50
- Misses 20 26 +6
|
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
Seems like there was an update to node.js (14 -> 16) & npm (6 -> 8) versions in github actions runners about 4 hours ago causing open-telemetry/opentelemetry-js#2093 to happen on lint task as it uses whatever node/npm is installed (CI run log) however adding |
OK. We should work on the latest anyway so i'll look into updating our dependencies to get it up and running. Thanks for looking into it. For now, we can probably use the setup-js action that the unit tests use to get around 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.
This seems like an awesome addition and it is very easy to follow since the API is so straightforward. I made a comment about the non-specified attributes but if we don't get any guidance from the TC i'm happy to move forward with this since it is 0.x anyways.
span.setAttribute('component', this.component); | ||
span.setAttribute('longtask.name', entry.name); | ||
span.setAttribute('longtask.entry_type', entry.entryType); | ||
span.setAttribute('longtask.duration', entry.duration); |
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 assume there is no spec for these attributes. Can we see if there is any guidance from the spec/TC on how we should handle custom or non-specified attributes? If these are ever specified in the future in a different format it could make us non-compliant.
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.
@dyladan instrumentation are not stable yet so this shouldn't be considered a breaking change if in the future they are in the spec ?
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.
No but in general I think we should try to be consistent.
Which problem is this PR solving?
This PR adds instrumentation for Long Tasks API (MDN, w3c), automatically generating spans when a task takes longer than 50ms (as of current long task API spec).
Instrumentation wise it's pretty simple - it creates a
PerformanceObserver
forlongtask
type (including any buffered = before instrumentation is started) and then creates spans dumping all of the data fromPerformanceLongTaskTiming
entriesAdding @mhennoch as possible component co-maintainer as we both handle rum at splunk and more likely at least one of us will be available for maintenance (eg Long Task API v2).
resolves open-telemetry/opentelemetry-js#1722