DOM event plugin add data-rum-id attribute #163

omerfarukaslan · 2022-05-20T17:55:50Z

Issue: As a user, I am unable to get accurate information about some user events when I render my page using React components. When we render certain components, they get rendered with children elements and we don't have access to those child elements to add id or any css locater at all. For example, when I render a it gets rendered like
<button id="myButton"><span>Click Here</span></button>, and when users click on span element, I see "SPAN" instead of "myButton" on the logs. We can use cssLocators for this but, we would need to add a separate event for each cssLocator which is a lot of manual work and increases code complexity.

Solution: I added an optional data-rum-id attribute to get logged within eventData as eventData.elementRumId. With this change DomEventPlugin will capture the data-rum-id from nearest parent element if the clicked element doesn't have one. And this will only work when a cssLocator is provided.

For example:
Interaction event:
{ event: 'click', cssLocator: '[label^="button-"]' }

Code:
<button id="button6" data-rum-id="button-six" label="button-label1"> Button Six </button>
<button id="button7" data-rum-id="button-seven" label="button-label2"> <span>Button Seven</span> </button>

Output
{"details":"{\"version\":\"1.0.0\",\"event\":\"click\",\"elementId\":\"button6\",\"cssLocator\":\"[label^=\\\"button-\\\"]\",\"elementRumId\":\"button-six\"}"}
{"details":"{\"version\":\"1.0.0\",\"event\":\"click\",\"elementId\":\"SPAN\",\"cssLocator\":\"[label^=\\\"button-\\\"]\",\"elementRumId\":\"button-seven\"}"}

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

limhjgrace · 2022-05-23T17:45:07Z

src/plugins/event-plugins/DomEventPlugin.ts

+                return firstMatchingElement.getAttribute('data-rum-id');
+            }
+        }
+        return '';


Can we return null so that we only add the elementRumId data to the eventData if an element with the data-rum-id attribute was found?

Could I know more about your use case? This implementation would require us to update the dom event type schema to add an additional field.

What are the performance implications if we were to look for all elements with the data-rum-id attribute for each element identified by the provided cssLocator ? Would this require looking through the entire DOM until one/the closest one is found?

Hello Grace, thank you for your time reviewing this.

There is a validation for this on line 133, this would return falsy for empty string and would not add elementRumId.
if (elementRumId) {

I would like to capture user clicks accurately on my website which uses React on the front end. Many of the UI libraries creates child elements while rendering the page. And as a developer I don't have access to these child elements. I am unable to provide an id or a css selector. When a users clicks on a button, I either get "SPAN" or a random id that was generated during the render instead of the id / cssLocator that I provided to the Button component. This is a common issue with the current structure. This PR would solve this problem.

This is a great call out. I would not expect a significant impact on the performance. event.composedPath() returns an array that lists parents of the clicked element vertically. However we can add a limit for this, let's say 5 levels above or something like that. Let me know what you think, please.

qhanam · 2022-05-23T20:16:44Z

src/plugins/event-plugins/DomEventPlugin.ts

@@ -129,13 +129,30 @@ export class DomEventPlugin implements Plugin {
            };
            if (cssLocator !== undefined) {
                eventData.cssLocator = cssLocator;
+                const elementRumId = this.getElementRumId(event, cssLocator);
+                if (elementRumId) {
+                    eventData.elementRumId = elementRumId;


issue: We'll need to add elementRumId (or simply rumId) to the dom-event.json schema, or it won't be ingested.

Right now we have data from two different sources going into the elementId field. I wonder if we should disambiguate these by (1) making elementId optional and only add it if an was found, and (2) adding an element field for the type of element.

The event would look like:

{ event: 'click', element: 'SPAN', cssLocator: 'button', rumId: 'button-12345' }

This is a good suggestion. I agree to both.

issue: We'll need to add elementRumId (or simply rumId) to the dom-event.json schema, or it won't be ingested.

It is added to that file. And I will rename it as rumId, it looks simpler and better.

qhanam · 2022-05-23T20:27:34Z

src/plugins/event-plugins/DomEventPlugin.ts

@@ -129,13 +129,30 @@ export class DomEventPlugin implements Plugin {
            };
            if (cssLocator !== undefined) {
                eventData.cssLocator = cssLocator;
+                const elementRumId = this.getElementRumId(event, cssLocator);


question: Why restrict this to css locators? For example, if I record all clicks on the document element, I may still want to use data-rum-id

Generalizing this would be even more preferable. In this case we could look up for the data-rum-id attribute to locate the firstMatchingElement.

qhanam · 2022-05-23T20:44:37Z

Overall I think this is a good solution.

One downside is that web application maintainers must an additional field data-rum-id. However, I think that a unique field is likely necessary in a similar way to why we need custom test ids -- ID fields may be auto-generated by the framework.

Before we merge this I think we should still do some due diligence to determine:

Is there an existing standard/best practice for identifying elements in a DOM event? If so, do we adhere to it?
Is data-rum-id an appropriate name for this attribute?

omerfarukaslan · 2022-05-23T22:55:27Z

Overall I think this is a good solution.

One downside is that web application maintainers must an additional field data-rum-id. However, I think that a unique field is likely necessary in a similar way to why we need custom test ids -- ID fields may be auto-generated by the framework.

Before we merge this I think we should still do some due diligence to determine:

Is there an existing standard/best practice for identifying elements in a DOM event? If so, do we adhere to it?

Is data-rum-id an appropriate name for this attribute?

I will take a look at this and post my findings here.
Attribute name could be anything, data-analytics-id, data-tracking-id, etc.

omerfarukaslan · 2022-05-23T23:52:08Z

Hey Quinn,
Thank you for taking time to review this. I have addressed your suggestions except these two:
Is there an existing standard/best practice for identifying elements in a DOM event? If so, do we adhere to it?
It looks like the general approach is to use event.target and this is how it's currently done on RUM. A second option would be allowing a recorder JS function something like cwr.putEvent(eventInformation, etc). I think we should work on this in future. Could be a nice implementation.

Attribute name could be anything, data-analytics-id, data-tracking-id, etc.
We can make changes to attribute name. Let me know however you would like to proceed.

Note: Unit and integrations tests are failing due to the elementId -> element change. I am unable to commit changes on the test files. It shows an error: __tests__/DomEventPlugin.test.ts' is not included in project.

qhanam · 2022-05-24T04:39:17Z

Thanks @omerfarukaslan !

A second option would be allowing a recorder JS function

So this would be a function that, given an Event object, returns an ID?

I like this solution because it offloads the responsibility to the application, so it is a general solution, and we could continue to use a single 'elementId' field.

The tradeoff is that it would require additional configuration by the application. This can be mitigated by:

Providing a sane default (the current behavior).
Documenting the ancestor-ID search approach in the 'id recorder' config documentation
(Optional) Providing an implementation of the ancestor search approach

Specifically, I'm thinking we could add this ID recorder to the DomEventPluginConfig

omerfarukaslan · 2022-05-24T16:54:56Z

So this would be a function that, given an Event object, returns an ID?

I was thinking something like a method that users manually call from their JS to record user events. Let's say I have a function, I would like to log a user event whenever it's executed. We could let them use a method like rum.putEvent("function executed",... some other args)

But for now, I really liked the idea of logging the Element, ElementId and RumId properties.

The only thing we could do on top of this is allowing users to enable/disable this feature from RUM config. Just like they do with the mutationObserver. What do you think?

omerfarukaslan · 2022-05-24T20:26:52Z

Thank you for your valued feedback. I added a configuration for enableRumIdTracker. So users can enable/disable ancestor search feature.

qhanam · 2022-05-25T05:20:53Z

Thanks @omerfarukaslan; we have a few good options here, but I'm not sure which is best.

I'd like to look into this use case a bit more before committing.

I think accepting a callback function which generates an ID will provide the most flexibility (users can make this function whatever they want) and safety (we won't do ancestor search by default). The question in my mind is, should we create new fields, or leave a single ID field. The tradeoff is that if we split the fields, it will be difficult to identify the correct 'id', but if we use a single field, we might loose information. Maybe we can do both somehow.

omerfarukaslan · 2022-05-25T17:10:59Z

should we create new fields, or leave a single ID field. The tradeoff is that if we split the fields, it will be difficult to identify the correct 'id', but if we use a single field, we might loose information. Maybe we can do both somehow.

I think using a single field is not really useful. Value type should be consistent. Currently is shows the node name for some records and element id for others. Using separate fields like element, elementId (optional) and rumId (optional) sounds like a better idea. It would make it easier to analyze the logs knowing what to expect from a certain field.

I think accepting a callback function which generates an ID will provide the most flexibility (users can make this function whatever they want) and safety (we won't do ancestor search by default).

We can certainly have users define & use their own functions through config. However when we try to extract some unique id from an Event, there are not many options that users could benefit from (other than nodeName, elementId or ancestor search). RUM is currently pretty straight forward to setup and use. I'm worrying about complicating it.

Please let me know what you think.

qhanam · 2022-05-25T17:31:13Z

Value type should be consistent. Currently is shows the node name for some records and element id for others. Using separate fields like element, elementId (optional) and rumId (optional) sounds like a better idea. It would make it easier to analyze the logs knowing what to expect from a certain field.

Good point, makes sense, I think I'm on board with this.

The only issue is that we're breaking backwards compatibility if we move the element type to its own field. However, I think the risk is low, and that it is worth doing now.

when we try to extract some unique id from an Event, there are not many options that users could benefit from (other than nodeName, elementId or ancestor search)

Even when searching ancestors, there are several ways I might do it. For example, I might want to use a different attribute name (i.e., other than data-rum-id), or I might want to get the ID of a specific element type (e.g., "give me the id of the closest button ancestor"), or I might want to get a CSS class.

I feel like accepting a function as a parameter, rather than hard-coding an ancestor search, also reduces the complexity from the perspective of the web client. It offloads the implementation to the application, so (1) if the application doesn't use it, they don't get the code, and (2) we don't need to maintain this when application owners need changes to it.

omerfarukaslan · 2022-05-26T19:50:18Z

ven when searching ancestors, there are several ways I might do it. For example, I might want to use a different attribute name (i.e., other than data-rum-id), or I might want to get the ID of a specific element type (e.g., "give me the id of the closest button ancestor"), or I might want to get a CSS class.

This is a valid point and I agree. That looks like a more flexible option. I added a callback function to the config named customIdTracker. And we can provide the rumId tracker as an example in the docs. Please let me know what you think.

private getRumId(event: Event): string {
    if (event.composedPath) {
        const eventPath = event.composedPath();
        const firstMatchingElement = eventPath.find(
            (element: Element) =>
                element.hasAttribute && element.hasAttribute('data-rum-id')
        ) as Element;

        if (firstMatchingElement) {
            return firstMatchingElement.getAttribute('data-rum-id');
        }
    }
    return '';
}

src/plugins/event-plugins/DomEventPlugin.ts

qhanam · 2022-05-31T21:34:25Z

src/plugins/event-plugins/DomEventPlugin.ts

    enableMutationObserver?: boolean;
    events: TargetDomEvent[];
 };

 const defaultConfig: DomEventPluginConfig = {
+    customIdTracker: undefined,


suggestion: For all config options, we define sane default. Define a default function here and have it return undefined. Then we can get rid of the if statement at line 140.

Suggested change

customIdTracker: undefined,

customIdTracker: () => undefined,

Sounds like a better approach. Will add this.

qhanam · 2022-05-31T21:35:22Z

src/plugins/event-plugins/DomEventPlugin.ts

    enableMutationObserver?: boolean;
    events?: TargetDomEvent[];
 };

 export type DomEventPluginConfig = {
+    customIdTracker?: (event: Event) => string;


Suggested change

customIdTracker?: (event: Event) => string;

customIdTracker: (event: Event) => string;

issue: We'll need to think of a good name for this. Maybe just customId? It violates verb-phrase method name convention, but will clearly connect to the customId property in the event schema.

I renamed this as getCustomId

qhanam · 2022-05-31T21:40:45Z

We'll also need tests before merging this PR. Unit tests are required. Integ tests are optional; i.e., if there are integrations we want to check.

omerfarukaslan · 2022-05-31T22:48:51Z

We'll also need tests before merging this PR. Unit tests are required. Integ tests are optional; i.e., if there are integrations we want to check.

I created unit tests but I'm unable to commit them. I see this lint error when I try to commit:
aws-rum-web/src/plugins/event-plugins/__integ__/DomEventPlugin.test.ts' is not included in project.
do you have any suggestions?

qhanam · 2022-05-31T23:18:14Z

I created unit tests but I'm unable to commit them. I see this lint error when I try to commit:
aws-rum-web/src/plugins/event-plugins/integ/DomEventPlugin.test.ts' is not included in project.
do you have any suggestions?

This might have been fixed by #165 ... is this still happening after merging?

omerfarukaslan · 2022-06-01T00:46:07Z

I created unit tests but I'm unable to commit them. I see this lint error when I try to commit:
aws-rum-web/src/plugins/event-plugins/integ/DomEventPlugin.test.ts' is not included in project.
do you have any suggestions?

This might have been fixed by #165 ... is this still happening after merging?

It worked after merging. Thank you! Your feedback is addressed. And I was able to add unit tests for the getCustomId() functionality.

qhanam · 2022-06-01T20:23:39Z

I've polished the change a bit.

Most notably, I've renamed both the config option and the event field name interactionId. In my mind the purpose of this field is to uniquely identify an interaction between the user and the UI. This also fits with our telemetry name interaction. I've used the same name -- interactionId for both the config option and the event field to make it clear how they are connected.

Before we merge the change, we will need to (1) update the schema in the back end, and (2) add documentation for this option.

@limhjgrace do these changes look ok to you?

qhanam · 2022-06-03T20:29:46Z

We are waiting for the new event schema to be deployed before merging.

omerfarukaslan · 2022-06-06T15:00:25Z

Thank you for the update! Would that be possible to provide an ECD for this feature, when is this going to be available in prod?

qhanam · 2022-06-08T15:30:59Z

@omerfarukaslan we're looking at around two weeks to get the back end sorted

omerfarukaslan · 2022-06-15T13:47:44Z

Hey @qhanam, thank you for the update. I'd like to share a better function that doesn't run on O(n), as a reference. This function will only check the nearest 5 ancestors, and break when attribute is found. The previously shared function was using JS find() method to check every single ancestor.

private getRumId(event: Event): string {
    if (event.composedPath) {
        const eventPath = event.composedPath();
        let firstMatchingElement: Element | null;

        for(let i=0; i<5; i++){
          const element = eventPath[i] as Element;
          if(element?.hasAttribute && element?.hasAttribute("data-rum-id")){
            firstMatchingElement = element;
            break;
          }
        }

        if (firstMatchingElement) {
            return firstMatchingElement.getAttribute('data-rum-id');
        }
    }
    return '';
}

omerfarukaslan · 2022-06-23T16:36:25Z

@omerfarukaslan we're looking at around two weeks to get the back end sorted

Hi @qhanam is there any update for this PR? Thank you!

limhjgrace · 2022-06-28T22:11:05Z

Hey @omerfarukaslan, we're still sorting out the backend and will most likely deploy the changes to the Web Client by the end of next week. Thanks for checking in and patiently waiting!

omerfarukaslan · 2022-06-28T23:26:23Z

Hey @omerfarukaslan, we're still sorting out the backend and will most likely deploy the changes to the Web Client by the end of next week. Thanks for checking in and patiently waiting!

Awesome!! Looking forward to it. Thank you for the update.

qhanam · 2022-07-06T19:29:51Z

It is safe to merge this now.

omerfarukaslan · 2022-07-06T19:43:00Z

It is safe to merge this now.

Awesome!! Cannot wait to see it in action. Thank you!

This PR adds documentation as part of changes introduced in #163.

This commit adds documentation as part of changes introduced in #163.

limhjgrace reviewed May 23, 2022

View reviewed changes

qhanam reviewed May 23, 2022

View reviewed changes

omerfarukaslan force-pushed the main branch from f45180a to 9b01f97 Compare May 23, 2022 23:42

omerfarukaslan force-pushed the main branch from 9b01f97 to 682db4b Compare May 24, 2022 20:23

qhanam reviewed May 31, 2022

View reviewed changes

src/plugins/event-plugins/DomEventPlugin.ts Outdated Show resolved Hide resolved

qhanam reviewed May 31, 2022

View reviewed changes

qhanam requested a review from limhjgrace June 1, 2022 20:20

limhjgrace approved these changes Jun 1, 2022

View reviewed changes

qhanam force-pushed the main branch from c3445b7 to 2912fdd Compare July 12, 2022 05:36

Add interaction id to DOM event plugin

ee8f548

qhanam force-pushed the main branch from 2912fdd to ee8f548 Compare July 12, 2022 16:54

qhanam merged commit 92eef3c into aws-observability:main Jul 12, 2022

limhjgrace added a commit that referenced this pull request Nov 18, 2022

docs: interactionId parameter for interaction

11ee118

This PR adds documentation as part of changes introduced in #163.

limhjgrace mentioned this pull request Nov 18, 2022

docs: interactionId parameter for interaction telemetry #297

Merged

qhanam pushed a commit that referenced this pull request Nov 23, 2022

docs: interactionId parameter for interaction

e184e42

This commit adds documentation as part of changes introduced in #163.

qhanam pushed a commit that referenced this pull request Nov 23, 2022

docs: interactionId parameter for interaction

822257c

This commit adds documentation as part of changes introduced in #163.

qhanam pushed a commit that referenced this pull request Nov 23, 2022

docs: interactionId parameter for interaction

8f3c39f

This commit adds documentation as part of changes introduced in #163.

limhjgrace added a commit that referenced this pull request Nov 23, 2022

docs: interactionId parameter for interaction configuration(#297)

6104e35

This commit adds documentation as part of changes introduced in #163.

	customIdTracker: undefined,
	customIdTracker: () => undefined,

	customIdTracker?: (event: Event) => string;
	customIdTracker: (event: Event) => string;

DOM event plugin add data-rum-id attribute #163

DOM event plugin add data-rum-id attribute #163

Conversation

omerfarukaslan commented May 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qhanam commented May 23, 2022

omerfarukaslan commented May 23, 2022

omerfarukaslan commented May 23, 2022

qhanam commented May 24, 2022

omerfarukaslan commented May 24, 2022

omerfarukaslan commented May 24, 2022

qhanam commented May 25, 2022

omerfarukaslan commented May 25, 2022

qhanam commented May 25, 2022

omerfarukaslan commented May 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qhanam May 31, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qhanam commented May 31, 2022

omerfarukaslan commented May 31, 2022

qhanam commented May 31, 2022

omerfarukaslan commented Jun 1, 2022

qhanam commented Jun 1, 2022 • edited

qhanam commented Jun 3, 2022

omerfarukaslan commented Jun 6, 2022

qhanam commented Jun 8, 2022

omerfarukaslan commented Jun 15, 2022

omerfarukaslan commented Jun 23, 2022

limhjgrace commented Jun 28, 2022

omerfarukaslan commented Jun 28, 2022

qhanam commented Jul 6, 2022

omerfarukaslan commented Jul 6, 2022

qhanam May 31, 2022 •

edited

qhanam commented Jun 1, 2022 •

edited