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

React: User Interaction only returns "Click - div" #969

Closed
reecebenson opened this issue Feb 19, 2021 · 22 comments · Fixed by #1155
Closed

React: User Interaction only returns "Click - div" #969

reecebenson opened this issue Feb 19, 2021 · 22 comments · Fixed by #1155
Assignees
Labels

Comments

@reecebenson
Copy link

reecebenson commented Feb 19, 2021

This issue is possibly related to the intended fix from PR #951, however applying the changes there do not resolve the below issue for me.

Whenever a target is clicked, the USER_INTERACTION event is fired. As an example button:

<button name="applyFiltersBtn" class="ui icon primary left labeled button">
  <i aria-hidden="true" class="check icon"></i>
  Apply
</button>

This then fires the following log:

[Elastic APM] startTransaction(86115b370489ca7c, Click - div, user-interaction)

As you can see, the name of this transaction is titled Click - div, even though the (what looks like) intended behaviour of performance-monitoring.js:189 is to grab the name attribute from the element, in this case applyFiltersBtn. I'd expect this transaction to be named Click - button["applyFiltersBtn"] and for this to be recorded in the APM under a "user-interaction", whilst capturing network traffic to include spans such as API requests, etc.

When doing some debugging, I logged out what was being passed to shouldInstrumentEvent (or from #951, I have this applied change, along with the other changes from the PR). When logging the target variable from the shouldInstrumentEvent function and also logging the target from the getEventTargetSub, in all cases both parts of the aforementioned give back one of the following:

  • <div id="root">
  • Window
  • <react><react/>.

It never gives back the actual target of (in the example above) a Button. Is this intentional, if so, how would one implement a transaction to follow up on user interactions with buttons and log them correctly in APM, with React?

@StefanoSega
Copy link

same issue ...
everything else works fine, but despite specifying a value for the name attribute in the buttons when triggered the log is always Click - div
I thought it was an issue of event bubbling but is not the case

@vigneshshanmugam
Copy link
Member

vigneshshanmugam commented Mar 5, 2021

We can fix this today by using the technique described here - https://github.com/elastic/apm-agent-rum-js/pull/951/files#r588056071. the resulting value of transaction would be Click - button["applyFiltersBtn"]

I would add this check to the PR and it should be good to go.

@reecebenson
Copy link
Author

We can fix this today by using the technique described here - https://github.com/elastic/apm-agent-rum-js/pull/951/files#r588056071. the resulting value of transaction would be Click - button["applyFiltersBtn"]

Sounds good, I'll give that a test with @clyfish's latest updates to that PR and report back here. Cheers for the update!

@reecebenson
Copy link
Author

With the latest updates by @clyfish, still seeing a similar output as to what is described in this issue. When clicking the button, I continue to see Click - div, user-interaction. When logging out the activeElement, I'm seeing <div id="root"> still.

log output
[test] target is not window, document or have activeElement <div id="root">
[test] target is not window, document or have activeElement <div id="root">
[Elastic APM] redefining transaction(d620f145002f6e68, Click - div, user-interaction) to (Click - div, user-interaction) Object { name: "Click - div", type: "user-interaction", options: {…}, id: "d620f145002f6e68", traceId: "cb63b3d1613816e2302c47f4a40f3c11", sampled: true, sampleRate: 1, timestamp: undefined, _start: 118446, _end: undefined, … }
[Elastic APM] redefining transaction(d620f145002f6e68, Click - div, user-interaction) to (Click - div, route-change) Object { name: "Click - div", type: "user-interaction", options: {…}, id: "d620f145002f6e68", traceId: "cb63b3d1613816e2302c47f4a40f3c11", sampled: true, sampleRate: 1, timestamp: undefined, _start: 118446, _end: undefined, … }

Updating this part of performance-monitoring.js to:

        let target = task.target
        if (
          target === window ||
          target === document ||
          (typeof target.contains === 'function' &&
            target.contains(document.activeElement))
        ) {
          target = document.activeElement
          console.log('[test] target redefined to', target);
        }
        else {
          console.log('[test] target is not window, document or have activeElement', target);
        }

@reecebenson
Copy link
Author

reecebenson commented Mar 5, 2021

In order to get the expected functionality of user-interaction to have the correctly focused element, I had to do a horrible workaround like this (for testing purposes):

document.addEventListener(
  'click',
  (e) => {
    // Get our clicked element
    const target = (e.target as HTMLElement) || null;
    if (!target) return;

    // Check if we have a valid element
    if (typeof target.getAttribute !== 'function') return;

    // Generate a Transaction Name from the attributes
    const type = target.tagName;
    const name = target.getAttribute('name') || '';
    const transactionName = `Click - ${type}["${name}"]`;

    // Set activeElement
    console.log('[redef]: custom evt click', transactionName);
    target.focus();
  },
  true
);

Which result in the following logs:

log output
[redef]: custom evt click Click - BUTTON.applyFiltersBtn["applyFiltersBtn"]
[Elastic APM] redefining transaction(53cee16f7cb42b3f, Click - body, user-interaction) to (Click - button["applyFiltersBtn"], user-interaction) Object { name: "Click - body", type: "user-interaction", options: {…}, id: "53cee16f7cb42b3f", traceId: "8fab728bd8264784c4711f12679ad580", sampled: true, sampleRate: 1, timestamp: undefined, _start: 287124, _end: undefined, … }
[Elastic APM] redefining transaction(53cee16f7cb42b3f, Click - button["applyFiltersBtn"], user-interaction) to (Click - button["applyFiltersBtn"], user-interaction) Object { name: "Click - button[\"applyFiltersBtn\"]", type: "user-interaction", options: {…}, id: "53cee16f7cb42b3f", traceId: "8fab728bd8264784c4711f12679ad580", sampled: true, sampleRate: 1, timestamp: undefined, _start: 287124, _end: undefined, … }
[Elastic APM] redefining transaction(53cee16f7cb42b3f, Click - button["applyFiltersBtn"], user-interaction) to (Click - button["applyFiltersBtn"], route-change) Object { name: "Click - button[\"applyFiltersBtn\"]", type: "user-interaction", options: {…}, id: "53cee16f7cb42b3f", traceId: "8fab728bd8264784c4711f12679ad580", sampled: true, sampleRate: 1, timestamp: undefined, _start: 287124, _end: undefined, … }
[Elastic APM] transaction(789344a1c1770ed2, Click - button["applyFiltersBtn"]) was discarded! Transaction does not have any spans

This looks like what I'm expecting (except the last log), however the final thing on my list is capturing network traffic. I'm not sure how this works in its entirety, as I cannot get it to record the API calls it is making. The logic of my applyFiltersBtn is somewhat as follows:

  1. User clicks the apply button
  2. Dispatch an action to refresh the table
  3. React Hook checks the refresh flag to see if it is set to true, and dispatches another action which subsequently calls an API.
  4. API response is received and the (Functional) Component is updated

Is there a way to force this network activity to be logged, instead of the transaction being discarded?

@clyfish
Copy link
Contributor

clyfish commented Mar 6, 2021

@reecebenson I've tested without your workaround, and it works on Chrome 89. What browser did you test on?

If there is an http-request transaction after the user-interaction transaction within 100ms, The user-interaction will not be discarded, because the user-interaction transaction has no spans itself and the http-request transaction has.

@reecebenson
Copy link
Author

reecebenson commented Mar 8, 2021

@reecebenson I've tested without your workaround, and it works on Chrome 89. What browser did you test on?

If there is an http-request transaction after the user-interaction transaction within 100ms, The user-interaction will not be discarded, because the user-interaction transaction has no spans itself and the http-request transaction has.

@clyfish I'm testing using Firefox 86, but still seeing <div id="root"> whilst testing without my workaround. We're using React 17.0.1, that I know has some breaking changes from v17 (namely the changes to event delegation). I haven't retested using Chrome (don't have it installed, unfortunately) but will try and get to that today.

Regarding the network activity, in my use case 100ms isn't enough buffer time to track network activity, as I have state changes to apply before firing something to the API. There doesn't look like there is a way to override this value either. I don't want to propose an increase in this value as it's probably a single use-case, but an override to this value would be beneficial for some. Is that something we could introduce here, possibly a new issue for tracking?

also, another question ...
after collecting all user interactions as transactions how I can see them in time order kinda like breadcrumbs, to understand exactly what the user did in sequence?

@StefanoSega Please create a new issue as your question is irrelevant to this issue. It makes these issues difficult to follow if conversation starts becoming off-topic.

@clyfish
Copy link
Contributor

clyfish commented Mar 8, 2021

I've tested on Firefox 86, and it still works without your workaround. Are there any blur or focus called in your code that resetting the activeElement to document.body?

https://developer.mozilla.org/en-US/docs/Web/API/HTMLOrForeignElement/blur

Regarding the network activity, in my use case 100ms isn't enough buffer time to track network activity, as I have state changes to apply before firing something to the API.

Why does a state change take more than 100ms? Normally, the request is initiated immediately after clicking.

@reecebenson
Copy link
Author

I've tested on Firefox 86, and it still works without your workaround. Are there any blur or focus called in your code that resetting the activeElement to document.body?

https://developer.mozilla.org/en-US/docs/Web/API/HTMLOrForeignElement/blur

Regarding the network activity, in my use case 100ms isn't enough buffer time to track network activity, as I have state changes to apply before firing something to the API.

Why does a state change take more than 100ms? Normally, the request is initiated immediately after clicking.

Having a look through, I haven't got any additional .blur() or .focus() calls apart from the workaround I provided above (that I've commented out for testing here). I've noticed a slight improvement to logs whilst routing throughout my app, where it now records Click - a and occassionally Click - body with the relevant network activity. Although, the button in question is still being classed as a div and then being discarded. In attempts to give you a bigger picture, the DOM is similar to that of the below around the button.applyFiltersBtn:

<div class="root">
  <div class="ui attached segment">
    <div>(some filtering content, dropdowns, inputs, etc.)</div>
    <button name="applyFiltersBtn" class="ui icon primary left labeled button">
      <i aria-hidden="true" class="check icon"></i>
      Apply
    </button>
  </div>
  <div>(the rest of the app, tables, etc.)</div>
</div>

Regarding the Click - a calls, this is some buttons on the table of the app. The reasoning as to why state change can take longer than 100ms is (partly) down to poor optimisation on the table (looking to lazy load this in the future / scroll load) but that is part of a future sprint as we're still in the building phase. When clicking Apply, the logic is to update the Redux store to be manipulated and prepare a payload to be pushed to the API (can contain quite a lot of data to query against the API). When that logic is generated and prepared, state is updated for a loading indicator and awaits a response. When monitoring this with the Network tab on devtools, the request happens around 500-600ms after the initial click.

@reecebenson
Copy link
Author

reecebenson commented Mar 8, 2021

I've been able to get this to work how I expect it to work, however can't seem to get it functioning exactly how I'd like due to some UI library restrictions (for example, some out of the box components don't expose the name attribute to the rendered DOM). To work around this, I use data-rum as additional props are passed through to the DOM. In attempts to clear up the workflow I have here, here's the logs below with two steps - clicking the 'Apply Filters' button, and then clicking another button to 'View Authors'.

First Step (Apply Filters)

log output info
[Elastic APM] startTransaction(d69a67afece3fbc9, Click - body, user-interaction) my custom event firing a transaction with a larger threshold (1000ms)
[Elastic APM] redefining transaction(d69a67afece3fbc9, Click - body, user-interaction) to (Click - BUTTON.applyFiltersBtn, user-interaction) Object { name: "Click - body", type: "user-interaction", options: {…}, id: "d69a67afece3fbc9", traceId: "0e4c032c861fcad61fc25584ca32da38", sampled: true, sampleRate: 1, timestamp: undefined, _start: 5510, _end: undefined, … }
[Elastic APM] redefining transaction(d69a67afece3fbc9, Click - BUTTON.applyFiltersBtn, user-interaction) to (Click - button["applyFiltersBtn"], user-interaction) Object { name: "Click - BUTTON.applyFiltersBtn", type: "user-interaction", options: {…}, id: "d69a67afece3fbc9", traceId: "0e4c032c861fcad61fc25584ca32da38", sampled: true, sampleRate: 1, timestamp: undefined, _start: 5510, _end: undefined, … } elastic renaming the transaction
[Elastic APM] redefining transaction(d69a67afece3fbc9, Click - button["applyFiltersBtn"], user-interaction) to (Click - button["applyFiltersBtn"], user-interaction) Object { name: "Click - button[\"applyFiltersBtn\"]", type: "user-interaction", options: {…}, id: "d69a67afece3fbc9", traceId: "0e4c032c861fcad61fc25584ca32da38", sampled: true, sampleRate: 1, timestamp: undefined, _start: 5510, _end: undefined, … } no change?
[Elastic APM] redefining transaction(d69a67afece3fbc9, Click - button["applyFiltersBtn"], user-interaction) to (Click - button["applyFiltersBtn"], route-change) Object { name: "Click - button[\"applyFiltersBtn\"]", type: "user-interaction", options: {…}, id: "d69a67afece3fbc9", traceId: "0e4c032c861fcad61fc25584ca32da38", sampled: true, sampleRate: 1, timestamp: undefined, _start: 5510, _end: undefined, … } changed transaction type from user-interaction to route-change (correct - as URL is updated via history.push())
[Elastic APM] startSpan(GET https://api.example.com/v1/data/, external) on transaction(d69a67afece3fbc9, Click - button["applyFiltersBtn"]) API request is found
[Elastic APM] endSpan(GET https://api.example.com/v1/data/, external) on transaction(d69a67afece3fbc9, Click - button["applyFiltersBtn"]) API request returned
[Elastic APM] end transaction(d69a67afece3fbc9, Click - button["applyFiltersBtn"], route-change) Object { name: "Click - button[\"applyFiltersBtn\"]", type: "route-change", options: {…}, id: "d69a67afece3fbc9", traceId: "0e4c032c861fcad61fc25584ca32da38", sampled: true, sampleRate: 1, timestamp: undefined, _start: 5510, _end: 7733, … } transaction ended for Apply button

Second Step (Narrow Search)

The below table represents the second action, for the purpose of this example this button is an
"Authors" button that subsequently fetches data from the API related to the current page.

log output info
[Elastic APM] startTransaction(f18802a86b82aedd, Click - body, user-interaction) elastic detected a click
[Elastic APM] redefining transaction(f18802a86b82aedd, Click - body, user-interaction) to (Click - A.authorsTab, user-interaction) Object { name: "Click - body", type: "user-interaction", options: {…}, id: "f18802a86b82aedd", traceId: "e404c7eef739580ea62cd9054599554a", sampled: true, sampleRate: 1, timestamp: undefined, _start: 12947, _end: undefined, … } my custom event handler has renamed the transaction to A.authorsTab as the name attribute was not found, so used data-rum attribute (see snippet below)
[Elastic APM] redefining transaction(f18802a86b82aedd, Click - A.authorsTab, user-interaction) to (Click - div, user-interaction) Object { name: "Click - A.authorsTab", type: "user-interaction", options: {…}, id: "f18802a86b82aedd", traceId: "e404c7eef739580ea62cd9054599554a", sampled: true, sampleRate: 1, timestamp: undefined, _start: 12947, _end: undefined, … } elastic inconveniently renames this back to a generic Click - div as the name attribute does not exist (UI library limitation)
[Elastic APM] redefining transaction(f18802a86b82aedd, Click - div, user-interaction) to (Click - div, user-interaction) Object { name: "Click - div", type: "user-interaction", options: {…}, id: "f18802a86b82aedd", traceId: "e404c7eef739580ea62cd9054599554a", sampled: true, sampleRate: 1, timestamp: undefined, _start: 12947, _end: undefined, … } no change
[Elastic APM] redefining transaction(f18802a86b82aedd, Click - div, user-interaction) to (Click - div, route-change) Object { name: "Click - div", type: "user-interaction", options: {…}, id: "f18802a86b82aedd", traceId: "e404c7eef739580ea62cd9054599554a", sampled: true, sampleRate: 1, timestamp: undefined, _start: 12947, _end: undefined, … } correct, route is updated
[Elastic APM] startSpan(GET https://api.example.com/v1/author-info/, external) on transaction(f18802a86b82aedd, Click - div) api request detected
[Elastic APM] endSpan(GET https://api.example.com/v1/author-info/, external) on transaction(f18802a86b82aedd, Click - div) api request ended
[Elastic APM] end transaction(f18802a86b82aedd, Click - div, route-change) Object { name: "Click - div", type: "route-change", options: {…}, id: "f18802a86b82aedd", traceId: "e404c7eef739580ea62cd9054599554a", sampled: true, sampleRate: 1, timestamp: undefined, _start: 12947, _end: 15842, … } transaction ended

​The snippet mentioned above, and previously in this ticket has been slightly updated and now looks like the below. As we aren't able to override the reuseThreshold directly (through some configuration), the next best thing was to fire a new transaction as my event gets called:

import { apm } from '@elastic/apm-rum';

// Elastic APM Services
const apmServices = apm.serviceFactory;
const trService = apmServices.getService('TransactionService');
document.addEventListener(
  'click',
  (e) => {
    // Get our clicked element
    const target = (e.target as HTMLElement) || null;
    if (!target) return;

    // Check if we have a valid element
    if (typeof target.getAttribute !== 'function') return;

    // Check if we have the "data-rum" attribute
    const type = target.tagName;
    const attr = target.getAttribute('data-rum');
    if (!attr) return;

    // Generate a Transaction Name from the attributes
    let transactionName = `Click - ${type}`;
    if (attr) transactionName += `.${attr}`;

    // Set activeElement
    target.focus();

    // Kick start our APM transaction so that Elastic can redefine it
    // Let the transaction be reused by APM so that we can monitor traffic
    trService.startTransaction(transactionName, 'user-interaction', {
      managed: true,
      canReuse: true,
      reuseThreshold: 1000
    } as ActualTransactionOptions) as Transaction;
  },
  true
);

The ActualTransactionOptions refers to a cover interface, as the reuseThreshold isn't a valid propery for the TransactionOptions interface.

interface ActualTransactionOptions extends TransactionOptions {
  managed?: boolean;
  startTime?: number;
  canReuse?: boolean;
  reuseThreshold?: number;
}

All of the above gives me the results I'm expecting, but due to Elastic renaming the transaction name when I attempt to get in the middle of it causes it to be too generic for logging purposes (Click - div). I'm guessing what comes out of this is:

  1. Can we have an override setting to look for a different attribute if name isn't valid?
  2. Can we have an override setting for the reuseThreshold TransactionOption?

@reecebenson
Copy link
Author

Hi @clyfish / @vigneshshanmugam, would the two aforementioned overrides be something you'd consider? I'd be happy to raise a PR if that is the case - maybe even just the reuseThreshold override as I've raised a PR and issue for the UI library I'm using (Semantic UI for React). Let me know your thoughts, happy to discuss.

@vigneshshanmugam
Copy link
Member

@reecebenson Thanks for the extensive test. I did a quick look around the new React 17 event system. It seems like React is registering two click events on the application root where the react app is mounted One with useCapture flag set to true and other with false.

As a result, I can see your test producing redefining transactions with no changes and also document.activeElement also not having the correct focus.

I am still doing some testing to figure out what would be the best solution here.

@clyfish Not sure why you closed the PR. Using window.onclick is easier, but it does not guarantee that

  • our listeners always fires before everyone elses
  • We have to also account for events that have been explicity called with e.stopPropagation or e.preventDefault.

So if we can fix it using eventtarget, it would be a better solution.

@clyfish
Copy link
Contributor

clyfish commented Mar 17, 2021

document.activeElement also not having the correct focus

I can't reproduce it.

<!doctype html>
<div><a href="javascript:">test1</a></div>
<p>test2</p>
<div><button>test3</button></div>
<script>
window.addEventListener('click', () => {
    console.log(document.activeElement)
}, true)
</script>

click test1: document.activeElement is the a.
click test2: document.activeElement is the body.
click test3: document.activeElement is the button.

Not sure why you closed the PR

Because I think I've solved my problem by using window.onclick, and I don't know which solution is better for apm-agent-rum-js. You can create another PR to solve this issue.

our listeners always fires before everyone elses
We have to also account for events that have been explicity called with e.stopPropagation or e.preventDefault.

We can listen on window with useCapture flag before react-dom's render to solve the two problem.

@hmdhk
Copy link
Contributor

hmdhk commented Mar 30, 2021

Re. capturing the network event for the user-interaction transaction, I believe there's an async operation that we're not capturing between the time the click happens and the time the network event is fired. I think having a small reproduction code would be useful (maybe in a separate issue as well), @reecebenson would you be able to push a simplified version of your code so we can reproduce this?

@vigneshshanmugam , Have you been able to reproduce the element attribute issue locally? If so I think sharing your code would be helpful to debug this.

@reecebenson
Copy link
Author

reecebenson commented Mar 30, 2021

I think having a small reproduction code would be useful (maybe in a separate issue as well), @reecebenson would you be able to push a simplified version of your code so we can reproduce this?

@jahtalab I'll see if I can get some time authorised and put aside to produce a reproducable codebase for you. It may not be authorised until this time next week, however, so apologies in advance.

@hmdhk
Copy link
Contributor

hmdhk commented Mar 30, 2021

No worries @reecebenson , I think even a very simple piece of code (could be a single html file with some JS) that essentially demonstrates your problem is very helpful. There's no need for the actual code.

@vigneshshanmugam
Copy link
Member

Have you been able to reproduce the element attribute issue locally? If so I think sharing your code would be helpful to debug this.

Updating our react-* packages to 17 and running our E2E application for React would showcase the problem. Also play a bit with this PR once you are done updating - #951. You will notice how the attribute changes.

@hmdhk
Copy link
Contributor

hmdhk commented Apr 14, 2021

@vigneshshanmugam , to post the examples he has generated in a PR so we can come up with solutions for this one.

@reecebenson
Copy link
Author

Hi @vigneshshanmugam / @jahtalab, apologies for not getting back to you with a snippet, but has there been any work on this that I could potentially test with?

@fsalazarh
Copy link

Hi, any updates with this Issue?

I'm using "@elastic/apm-rum": "^5.9.1" and i'm facing the same issue.

I'm created a React app using "react": "^17.0.1" with Ionic Framework, and I'm receive the Click - div, Click - ion-button and Click - ion-tab-button, Click - ion-content, etc... in the "user-interaction" transactions type.

@stasgm
Copy link

stasgm commented Aug 26, 2021

I'm created a React app using "react": "^17.0.1" with Ionic Framework, and I'm receive the Click - div, Click - ion-button and Click - ion-tab-button, Click - ion-content, etc... in the "user-interaction" transactions type.

I have the same problem with Vuetify. Not very meaningful name

@FedeOmoto
Copy link

Hi, any updates with this Issue?

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 a pull request may close this issue.

10 participants