-
Notifications
You must be signed in to change notification settings - Fork 134
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
✨ [RUM-6956] Add action name source #3115
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3115 +/- ##
==========================================
- Coverage 93.63% 93.17% -0.46%
==========================================
Files 276 276
Lines 7620 7638 +18
Branches 1711 1716 +5
==========================================
- Hits 7135 7117 -18
- Misses 485 521 +36 ☔ View full report in Codecov by Sentry. |
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
/to-staging |
🚂 Branch Integration: starting soon, median merge time is 10m Commit d99b471eff will soon be integrated into staging-45. Use |
…g-45 Integrated commit sha: d99b471 Co-authored-by: cy-moi <[email protected]>
Commit d99b471eff has been merged into staging-45 in merge commit abd1756a7f. Check out the triggered pipeline on Gitlab 🦊 |
|
/to-staging |
Devflow running:
|
…g-46 Integrated commit sha: d99b471 Co-authored-by: cy-moi <[email protected]>
d99b471
to
83c70f9
Compare
83c70f9
to
ce77be2
Compare
|
||
type ActionName = { | ||
name: string | ||
namingSource: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 suggestion: rename namingSource
to nameSource
to match the RUM event property name
💬 suggestion: improve the type of the source. Maybe use a const enum for it:
const enum ActionNameSource {
CUSTOM_ATTRIBUTE = 'custom_attribute',
...
}
) | ||
if (standardName) { | ||
return { name: standardName, namingSource: 'standard_attribute' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 suggestion: priorityStrategies
don't always return standard attribute values though, sometimes it is text_content
. Maybe strategies can return an ActionName
object, that we can use to get both the name and correct source.
(Sorry if this is what you asked confirmation for during the standup, I might have misunderstood)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I have a local version that manually returns different sources from each strategy. But I wonder if we could also make getTextualContent
return ActionName
object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning an ActionName object from getTextualContent sounds great. Maybe rename the function to getActionNameFromTextContent
or something, it could be more explicit
name: fallbackName, | ||
namingSource: 'text_content', | ||
} | ||
: { name: '', namingSource: 'blank_placeholder' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💬 suggestion: maybe just blank
could be simpler than blank_placeholder
Motivation
We want to add a field to indicate from which source the action names are generated.
Changes
Add a
name_source
field in@_dd.action
objectTesting
I have gone over the contributing documentation.