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

[WIP] feat(user-interaction): create one span per interaction #980

Closed

Conversation

bradfrosty
Copy link

@bradfrosty bradfrosty commented Apr 25, 2022

Which problem is this PR solving?

Fixes #548

This a WIP solution that replaces #548, and should account for some of the debate around the approach in that PR regarding heuristics. However, this solution is a much larger diff (albeit less code/simpler than existing solution by avoiding Zone patches).

See StackBlitz for example

  • Deduplicates user interaction spans per event target, rather than create one span per listener callback. Contrary to feat(user-interaction): deduplicate interaction spans by grouping them #743, this implementation does not rely on heuristics and accurately captures the execution context across multiple event handlers.
  • Further aligns the instrumentation with the upcoming Responsiveness CWV metric. This will be baked in the PerformanceObserver in the future where we can get increased accuracy on span timestamps. However, the metrics they record will not account for async tasks due to the difficult implementation, even though they would prefer that. Because this solution is leveraging Zone.js, we are capable of capturing this powerful data today.

Short description of the changes

  • Refactors Zone patching logic to a ZoneSpec implementation that performs async task tracking across all listeners for an event target, and provides onComplete/onError callbacks consumable from the instrumentation.
  • Changes span duration characteristics. It now starts from the first event.timeStamp, and ends after all async tasks have been performed within all listener callbacks. Starting from event.timeStamp improves the recording accuracy as it now accounts for the input delay (aka First Input Delay but for all interactions)
  • Calculates additional aggregates and events that may be useful when analyzing interactions, such as processingStart (when the browser is able to start executing), processingEnd (when all synchronous work is complete), listenerCount (total count of listeners that executed for the interaction), and detailed task counts (number of async tasks executed). The processingStart and processingEnd metrics are the same values found on the spec'd PerformanceEventTiming interface.
  • Silently records errors/exceptions on the interaction span without disrupting user-land error handling

Checklist

  • Ran npm run test-all-versions for the edited package(s) on the latest commit if applicable.
  • Add several new test cases (but having difficulty getting Karma to work right now)
  • Explore additional aggregates such as longest event handler within interaction, total interaction latency over time, average interaction latency over time, high quantile aggregates
  • Explore grouping up and down metrics (e.g. mousedown + mouseup, keydown + keyup) to further align with Responsiveness metric, since both events are guaranteed to occur and count as a single "interaction"
  • Check if ZoneContextManager is being used rather assume it is being used by checking window.Zone
  • Verify accuracy by comparing with PerformanceTimeline values

This refactors the user interaction instrumentation to create a
single span per event target. Previously it was creating one span
per listener function. This resulted in difficulties when analyzing
these spans, as they appeared idential.

This changes makes the instrumentation much more in line with the
upcoming Responsiveness metric: https://web.dev/better-responsiveness-metric/

In order to accomplish this, all patching of the Zone APIs have been removed.
Instead, it uses the addEventListener/removeEventListener patches
just like the non-Zone implementation. Instead, it leverages a new
ZoneSpec definition to manage interaction state.
@bradfrosty bradfrosty force-pushed the enhance-user-interaction branch from 8d9714e to 51cae2e Compare April 25, 2022 13:48
// current === target is true only when intercepting "root" UserInteractionZone
if (current === target) {
try {
// this.printDebugInfo('root::onInvoke');
Copy link
Contributor

Choose a reason for hiding this comment

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

snip? Or useful to keep in for debugging?

constructor(config?: UserInteractionInstrumentationConfig) {
super('@opentelemetry/instrumentation-user-interaction', VERSION, config);
this._eventNames = new Set(config?.eventNames ?? DEFAULT_EVENT_NAMES);
this._shouldPreventSpanCreation =

Choose a reason for hiding this comment

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

why did we get rid of the shouldPreventSpanCreation?

new UserInteractionZoneSpec(
span,
function onComplete(attributes, events) {
console.log('onComplete', event);

Choose a reason for hiding this comment

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

When using this w/ react, I'm noticing that this onComplete function is executed right away and isn't being delayed until the async activity is completed.

I updated your initial stack blitz with an example here if you'd like to take a look.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2022

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.

@github-actions github-actions bot added the stale label Jul 4, 2022
@github-actions
Copy link
Contributor

This PR was closed because it has been stale for 14 days with no activity.

@github-actions github-actions bot closed this Jul 25, 2022
@valerybugakov
Copy link
Contributor

Hey @bradfrosty, thanks for sharing this work! I just recently started looking into this package and bumped into similar issues. Have you wholly switched to maintaining your forked internal version of user-interactions instrumentation?

Let me know if I can help with anything here. I'd like to see this merged.
cc @stephencranedesign if you have thoughts on how to push this forward.

@valerybugakov
Copy link
Contributor

@dyladan, @rauno56, maybe?

@Josh-a-e
Copy link

Has this fix/change been abandoned?

@dyladan
Copy link
Member

dyladan commented Sep 16, 2022

@Josh-a-e this component was maintained by a contributor who is no longer with the project, but if you're interested in this I would suggest you join the RUM SIG. We're holding off on many browser things while we wait for them to make some fundamental decisions about how they should work, including things like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User events getting registered twice
7 participants