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

fix(@opentelemetry/instrumentation-user-interaction): add event listeners for each event on init #1749

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

pkanal
Copy link
Contributor

@pkanal pkanal commented Oct 20, 2023

Which problem is this PR solving?

Short description of the changes

  • Loop over event names and add event listeners for each in the init function.
  • Call the init function in the constructor

@pkanal pkanal changed the title add event listeners for each event on init fix(@opentelemetry/instrumentation-user-interaction): add event listeners for each event on init Oct 20, 2023
@pkanal pkanal added the bug Something isn't working label Oct 20, 2023
@codecov
Copy link

codecov bot commented Oct 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.03%. Comparing base (f65f2f1) to head (9fb5078).
Report is 188 commits behind head on main.

Current head 9fb5078 differs from pull request most recent head 7fed08b

Please upload reports for the commit 7fed08b to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1749      +/-   ##
==========================================
- Coverage   91.51%   91.03%   -0.49%     
==========================================
  Files         145      134      -11     
  Lines        7427     6683     -744     
  Branches     1486     1333     -153     
==========================================
- Hits         6797     6084     -713     
+ Misses        630      599      -31     

see 18 files with indirect coverage changes

@pkanal pkanal marked this pull request as ready for review October 30, 2023 15:04
@pkanal pkanal requested a review from a team as a code owner October 30, 2023 15:04
@martinkuba
Copy link
Contributor

The behavior is intended to generate spans only for events that have handlers in the application code. The handlers in application code will generally invoke other work (e.g. HTTP requests), which then make the interactions more significant (longer duration, part of a bigger trace, applicable to application logic) than just tracking all events that happen on the page. If there is a use case to track all events, I would suggest we add a new instrumentation that does not rely on Zone.js.

@github-actions github-actions bot requested a review from obecny January 22, 2024 13:42
tbrockman added a commit to tbrockman/browser-extension-for-opentelemetry that referenced this pull request May 31, 2024
* Changes from relying on externally_connectable to CustomEvents (which will eventually work in Firefox)
* Adds patch to @opentelemetry/instrumentation-user-interaction so specified event listeners are always registered, regardless of whether any other page code has already registered listeners (see:
open-telemetry/opentelemetry-js-contrib#1749)
* Adds .github workflow files for (some) CI/CD
* Updates README.md
@JamieDanielson
Copy link
Member

My understanding of this based on the description and Martin's comment above is this:

  • The instrumentation is expected to instrument handlers added in app code
  • This PR aims to add more listeners to be instrumented, without needing to add listeners to your app code

In that case I think it may be a feature instead of a bug fix, but I'm not super familiar with this instrumentation. Either way it seems this has been stalled for a bit. Is this update still something you're interested in moving forward @pkanal or is there another approach that is working for what you need?

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

Successfully merging this pull request may close these issues.

[@opentelemetry/instrumentation-user-interaction] No spans created without Zone.js
4 participants