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

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

Merged
merged 1 commit into from
Jul 12, 2022

Conversation

omerfarukaslan
Copy link
Contributor

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.

return firstMatchingElement.getAttribute('data-rum-id');
}
}
return '';
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. 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?
  2. 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.
  3. 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello Grace, thank you for your time reviewing this.

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

  2. 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.

  3. 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.

@@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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'
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good suggestion. I agree to both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Member

qhanam commented May 23, 2022

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:

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

@omerfarukaslan
Copy link
Contributor Author

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:

  1. Is there an existing standard/best practice for identifying elements in a DOM event? If so, do we adhere to it?
  2. Is data-rum-id an appropriate name for this attribute?
  1. I will take a look at this and post my findings here.
  2. Attribute name could be anything, data-analytics-id, data-tracking-id, etc.

@omerfarukaslan
Copy link
Contributor Author

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
Copy link
Member

qhanam commented May 24, 2022

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:

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

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

@omerfarukaslan
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@qhanam
Copy link
Member

qhanam commented May 25, 2022

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
Copy link
Contributor Author

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
Copy link
Member

qhanam commented May 25, 2022

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
Copy link
Contributor Author

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 '';
}

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

const defaultConfig: DomEventPluginConfig = {
customIdTracker: undefined,
Copy link
Member

Choose a reason for hiding this comment

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

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,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like a better approach. Will add this.

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

export type DomEventPluginConfig = {
customIdTracker?: (event: Event) => string;
Copy link
Member

@qhanam qhanam May 31, 2022

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed this as getCustomId

@qhanam
Copy link
Member

qhanam commented May 31, 2022

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
Copy link
Contributor Author

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
Copy link
Member

qhanam commented May 31, 2022

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
Copy link
Contributor Author

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 qhanam requested a review from limhjgrace June 1, 2022 20:20
@qhanam
Copy link
Member

qhanam commented Jun 1, 2022

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
Copy link
Member

qhanam commented Jun 3, 2022

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

@omerfarukaslan
Copy link
Contributor Author

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
Copy link
Member

qhanam commented Jun 8, 2022

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

@omerfarukaslan
Copy link
Contributor Author

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
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Member

qhanam commented Jul 6, 2022

It is safe to merge this now.

@omerfarukaslan
Copy link
Contributor Author

It is safe to merge this now.

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

@qhanam qhanam merged commit 92eef3c into aws-observability:main Jul 12, 2022
limhjgrace added a commit that referenced this pull request Nov 18, 2022
This PR adds documentation as part of changes introduced in #163.
qhanam pushed a commit that referenced this pull request Nov 23, 2022
This commit adds documentation as part of changes introduced in #163.
qhanam pushed a commit that referenced this pull request Nov 23, 2022
This commit adds documentation as part of changes introduced in #163.
qhanam pushed a commit that referenced this pull request Nov 23, 2022
This commit adds documentation as part of changes introduced in #163.
limhjgrace added a commit that referenced this pull request Nov 23, 2022
This commit adds documentation as part of changes introduced in #163.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants