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

✨ [RUMF-945] allow users to customize the attribute used to define the action name #919

Merged
merged 7 commits into from
Jul 1, 2021

Conversation

bcaudan
Copy link
Contributor

@bcaudan bcaudan commented Jun 30, 2021

Motivation

It can be useful to reuse existing tagging in the html to retrieve the action name rather than forcing customer to duplicate their tagging for every third party that need to retrieve this info.

take over of #903

Changes

Introduce new actionNameAttribute init configuration option

Usage:

<script>
  DD_RUM.init({
    ...
    trackInteractions: true,
    actionNameAttribute: 'data-custom-name'
    ...
  })
</script>

<a onclick="triggerSomeRequestOrDomChange()" data-custom-name="Login button">Try it out!</a>

Both data-dd-action-name and configured attribute can be used, data-dd-action-name is favored when both are present on an element.

Testing

unit, locally


I have gone over the contributing documentation.

@bcaudan bcaudan requested a review from a team as a code owner June 30, 2021 09:16
@bcaudan bcaudan changed the title ✨ allow users to customize the attribute used to define the action name ✨ [RUMF-945] allow users to customize the attribute used to define the action name Jun 30, 2021
@@ -42,6 +42,7 @@ export interface UserConfiguration {
publicApiKey?: string // deprecated
clientToken: string
applicationId?: string
actionNameAttribute?: string
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if the name is not misleading.
It can be confused with the name of the action object we collect
Maybe something like this is clearer:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we will add some doc about it to explain the usage so IMO it would be clearer enough.

@hdelaby @BenoitZugmeyer @webNeat any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

About the wording: actionNameAttribute + doc LGTM, it's simple enough.

About the configuration: maybe we could have a more abstract getActionName(element, event) function instead, to allow the customer to implement their own logic to add beside our logic? That would be a bit more flexible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a more abstract strategy sounds interesting but I'd be in favor of waiting to have the need, in order to see what could be the most relevant approach to do that.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, good to me too :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking that having an array of attributes to check for, instead of just one would offer more flexibility, but the function getActionName also is interesting.
The current solution sounds enough to me for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

The function idea is great! Love that concept.

@bcaudan bcaudan requested a review from BenoitZugmeyer June 30, 2021 13:02
@@ -42,6 +42,7 @@ export interface UserConfiguration {
publicApiKey?: string // deprecated
clientToken: string
applicationId?: string
actionNameAttribute?: string
Copy link
Member

Choose a reason for hiding this comment

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

About the wording: actionNameAttribute + doc LGTM, it's simple enough.

About the configuration: maybe we could have a more abstract getActionName(element, event) function instead, to allow the customer to implement their own logic to add beside our logic? That would be a bit more flexible.

@codecov-commenter
Copy link

Codecov Report

Merging #919 (f3c7736) into main (8084b2c) will decrease coverage by 0.01%.
The diff coverage is 76.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #919      +/-   ##
==========================================
- Coverage   89.10%   89.09%   -0.02%     
==========================================
  Files          81       81              
  Lines        3838     3841       +3     
  Branches      855      857       +2     
==========================================
+ Hits         3420     3422       +2     
- Misses        418      419       +1     
Impacted Files Coverage Δ
...ain/rumEventsCollection/action/actionCollection.ts 95.23% <0.00%> (ø)
packages/core/src/domain/configuration.ts 93.33% <50.00%> (-3.10%) ⬇️
...ventsCollection/action/getActionNameFromElement.ts 87.27% <87.50%> (+0.11%) ⬆️
.../domain/rumEventsCollection/action/trackActions.ts 97.87% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8084b2c...f3c7736. Read the comment docs.

Copy link
Member

@BenoitZugmeyer BenoitZugmeyer left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor

@webNeat webNeat left a comment

Choose a reason for hiding this comment

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

LGTM, just added a small question.

// Proceed to get the action name in two steps:
// * first, get the name programmatically, explicitly defined by the user.
// * then, use strategies that are known to return good results. Those strategies will be used on
// the element and a few parents, but it's likely that they won't succeed at all.
// * if no name is found this way, use strategies returning less accurate names as a fallback.
// Those are much likely to succeed.
return (
getActionNameFromElementProgrammatically(element) ||
getActionNameFromElementProgrammatically(element, DEFAULT_PROGRAMMATIC_ATTRIBUTE) ||
(userProgrammaticAttribute && getActionNameFromElementProgrammatically(element, userProgrammaticAttribute)) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explain why the default attribute has priority over the user custom attribute?

Copy link
Contributor Author

@bcaudan bcaudan Jul 1, 2021

Choose a reason for hiding this comment

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

it seemed more convenient for reusing a previous tagging but allow to override some elements with our own attribute if needed

@@ -42,6 +42,7 @@ export interface UserConfiguration {
publicApiKey?: string // deprecated
clientToken: string
applicationId?: string
actionNameAttribute?: string
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking that having an array of attributes to check for, instead of just one would offer more flexibility, but the function getActionName also is interesting.
The current solution sounds enough to me for now.

@bcaudan bcaudan merged commit 3000fc5 into main Jul 1, 2021
@bcaudan bcaudan deleted the niieani-bb/data-test-id branch July 1, 2021 08:55
ThibautGeriz pushed a commit that referenced this pull request Jul 1, 2021
…e action name (#919)

* ✨ allow users to customize the attribute used to define the action name

* fix lint/format issues

* style tweaks

* 🐛 fix userConfig -> config

* ✨ allow to have both data-dd-action-name et user configured attribute

Co-authored-by: Bazyli Brzóska <bazyli.brzoska@gmail.com>
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.

6 participants