-
Notifications
You must be signed in to change notification settings - Fork 144
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-5590] Add telemetry for INP null target #2895
Conversation
7fa099a
to
2f089aa
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2895 +/- ##
==========================================
- Coverage 93.67% 93.63% -0.04%
==========================================
Files 268 268
Lines 7584 7592 +8
Branches 1686 1691 +5
==========================================
+ Hits 7104 7109 +5
- Misses 480 483 +3 ☔ View full report in Codecov by Sentry. |
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
@@ -74,6 +73,10 @@ export function trackInteractionToNextPaint( | |||
configuration.actionNameAttribute | |||
) | |||
} else { | |||
addTelemetryDebug('INP target is null or not an element node', { | |||
target: JSON.stringify(newInteraction.target), // if target is not an element node, log it to help debugging |
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.
🔨 warning: JSON.stringify(newInteraction.target)
will always return {}
for a DOM node. Maybe you could do it like so:
{
hasTarget: !!newInteraction.target
targetIsConnected: newInteraction.target?.isConnected,
...
}
Also don't you want to know if an element node is connected or nor?
7cbc04b
to
3d72169
Compare
ce5e769
to
c031764
Compare
newInteraction.target, | ||
configuration.actionNameAttribute | ||
) | ||
addTelemetryDebug('INP target is null or not an element node', { |
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: To reduce the number of telemetry, you could collect it only when there is no interactionToNextPaintTargetSelector
.
Also collecting the telemetry on org2 could be enough. So you could use a feature flag.
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.
LGTM
packages/rum-core/src/domain/view/viewMetrics/trackInteractionToNextPaint.ts
Outdated
Show resolved
Hide resolved
if (inpTarget && isElementNode(inpTarget)) { | ||
interactionToNextPaintTargetSelector = getSelectorFromElement(inpTarget, configuration.actionNameAttribute) | ||
if (!interactionToNextPaintTargetSelector && isInpEnabled) { | ||
addTelemetryDebug('INP target selector is null', { | ||
targetIsConnected: inpTarget.isConnected, | ||
inp: newInteraction.duration, | ||
}) | ||
} | ||
} else { | ||
if (isInpEnabled) { | ||
addTelemetryDebug('INP target is null or not an element node', { | ||
hasTarget: !!inpTarget, | ||
targetIsConnected: inpTarget ? inpTarget.isConnected : null, | ||
inp: newInteraction.duration, | ||
}) | ||
} | ||
|
||
interactionToNextPaintTargetSelector = undefined |
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: to simplify a little bit:
if (inpTarget && isElementNode(inpTarget)) {
interactionToNextPaintTargetSelector = getSelectorFromElement(inpTarget, configuration.actionNameAttribute)
} else {
interactionToNextPaintTargetSelector = undefined
}
if (!interactionToNextPaintTargetSelector && isExperimentalFeatureEnabled(ExperimentalFeature.NULL_INP_TELEMETRY)) {
addTelemetryDebug("Fail to get INP target selector", {
hasTarget: !!inpTarget,
targetIsConnected: inpTarget ? inpTarget.isConnected : undefined,
targetIsElementNode: inpTarget ? isElementNode(inpTarget) : undefined,
inp: newInteraction.duration
})
}
Also collect isElementNode
so we cover the whole condition (inpTarget && isElementNode(inpTarget)
)
Motivation
Add telemetry for INP null target to get the impact.
Changes
Add telemetry debug
Testing
I have gone over the contributing documentation.