-
Notifications
You must be signed in to change notification settings - Fork 497
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 ability to disable span events #2188
feat(instrumentation-document-load): Add ability to disable span events #2188
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2188 +/- ##
==========================================
- Coverage 90.97% 90.38% -0.60%
==========================================
Files 146 148 +2
Lines 7492 7509 +17
Branches 1502 1573 +71
==========================================
- Hits 6816 6787 -29
- Misses 676 722 +46 |
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, added one minor comment about documentation of the new options
I am not sure how a real span looks like and if it can be helpful to enumarte the events that might be recorded/ignored by each option, thus helping users decide if they want each one on or off. But this can also be addressed separately later if there is motivation.
ignoreNetworkEvents?: boolean; | ||
ignorePerformanceEvents?: boolean; |
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.
Please add a section in this package README.md to document these options for package users, like the one for applyCustomAttributesOnSpan
We can also include a comment here to help users understand what each option does and when and why to use it. Similar to the comment few lines above the applyCustomAttributesOnSpan?:
option.
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 think the names are misleading, as both are provided by Performance API. Ideally they should be called ignorePerformanceNavigationEvents
and ignorePerformancePaintEvents
. Since the name ignoreNetworkEvents
has been already used in xhr and fetch instrumentation configs, I suggest changing at least the latter.
ignoreNetworkEvents?: boolean; | ||
ignorePerformanceEvents?: boolean; |
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 think the names are misleading, as both are provided by Performance API. Ideally they should be called ignorePerformanceNavigationEvents
and ignorePerformancePaintEvents
. Since the name ignoreNetworkEvents
has been already used in xhr and fetch instrumentation configs, I suggest changing at least the latter.
|
||
if (!this._getConfig().ignorePerformanceEvents) { | ||
addSpanPerformancePaintEvents(rootSpan); | ||
} |
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.
There is a way to do better and avoid calling the Performance API altogether. The function getPerformanceNavigationEntries
and in utils.js could return an empty entries
map when this flag is turned off.
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 think doing it this way makes it very explicit in the code where this option is being used which is why I would prefer to do it this way. This function will not get called if ignorePerformanceEvents
is set to true
so the Performance API won't be called this way either.
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.
Sorry I put this comment in the wrong place. I meant it can be avoided even here - https://github.com/honeycombio/opentelemetry-js-contrib/blob/feat/user-interaction-instrumentation-span-events/plugins/web/opentelemetry-instrumentation-document-load/src/instrumentation.ts#L103
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.
It looks like the entries
variable is actively used by the _startSpan
function to determine the start time so I'm inclined to leave this as is.
It looks like the changes suggested have been implemented. @pkanal is there anything else you wanted to add here? If not I think this is good to go 🚀 |
Which problem is this PR solving?
Adds the option to turn off sending span events generated by the document load instrumentation.
This is useful because this instrumentation can generate 8-10 span events per span which can become noisy. There is precedent for this API in the
instrumentation-fetch
andinstrumentation-xml-http
packages so I decided to keep it consistent with that API.Short description of the changes
ignoreNetworkEvents
optional option. If set totrue
, does not send network span events on any spans.ignorePerformanceEvents
optional option. If set totrue
, does not send performance events span events.Open questions
documentLoad
events but notresourceFetch
events)