PANA-5681: Integrate HeatmapIdentifierRegistry into RUM for heatmaps [3 of 4]#3205
PANA-5681: Integrate HeatmapIdentifierRegistry into RUM for heatmaps [3 of 4]#3205gonzalezreal wants to merge 1 commit into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 087cbc56d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
gonzalezreal
force-pushed
the
gonzalezreal/PANA-5681/rum-heatmaps
branch
from
7c039dc to
e76417d
Compare
This comment has been minimized.
This comment has been minimized.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/heatmaps #3205 +/- ##
====================================================
+ Coverage 72.09% 72.10% +0.01%
====================================================
Files 964 965 +1
Lines 35429 35492 +63
Branches 5880 5888 +8
====================================================
+ Hits 25542 25591 +49
- Misses 8278 8286 +8
- Partials 1609 1615 +6
🚀 New features to boost your workflow:
|
jonathanmos
changed the title
jonathanmos
force-pushed
the
gonzalezreal/PANA-5681/schema-updates
branch
from
ec4040e to
81e4f04
Compare
jonathanmos
force-pushed
the
gonzalezreal/PANA-5681/rum-heatmaps
branch
2 times, most recently
from
cbfbf76 to
9a2e330
Compare
jonathanmos
changed the title
jonathanmos
force-pushed
the
gonzalezreal/PANA-5681/rum-heatmaps
branch
from
9a2e330 to
e63b7f4
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
| val featureSdkCore = sdkCore as? FeatureSdkCore ?: return null | ||
| return featureSdkCore |
There was a problem hiding this comment.
I think we should always be able to perform this cast, so can drop the nullable:
| val featureSdkCore = sdkCore as? FeatureSdkCore ?: return null | |
| return featureSdkCore | |
| return (sdkCore as FeatureSdkCore) |
| * Replaces the current snapshot with [identifiers], scoped to [screenName]. | ||
| * | ||
| * @param identifiers a map of [android.view.View.getId] values to their [HeatmapIdentifier]s, | ||
| * @param identifiers a map of [System.identityHashCode] values to their [HeatmapIdentifier]s, |
There was a problem hiding this comment.
I think @aleksandr-gringauz had a point regarding identityHashCode usage (that there is a clash after certain number of objects, since it is re-used), but since we use it in the Session Replay for the object identity, I guess this is fine.
There was a problem hiding this comment.
yes, that is actually the reason I chose to use it here. We don't expect collisions on the same screen, but if we reach a conclusion that there is an issue we could append the hashcode of the direct parent for an even lower collision probability
There was a problem hiding this comment.
Yes. See this thread.
We don't expect collisions on the same screen
I didn't dig deep into how the identifier obtained using identityHashCode is used in this PR.
However in general, it is important to understand the following.
2 instances of the same type that currently have strong references in code can have the same identityHashCode. And the probability of this is higher than you might expect. This is what I explain in my comment.
The questions are:
- What are the requirements here for not having collisions? Do we have to avoid collisions during the lifetime of a single RUM View?
- And what is the number of "tries"? Is it the number of
Viewinstances that ever existed during the lifetime of the RUM View?
| internal var displayInfoProvider: InfoProvider<DisplayInfo> = NoOpDisplayInfoProvider() | ||
| internal val rumContextUpdateReceivers = mutableSetOf<FeatureContextUpdateReceiver>() | ||
| internal var insightsCollector: InsightsCollector = NoOpInsightsCollector() | ||
| override val heatmapIdentifierRegistry: HeatmapIdentifierRegistry by lazy { HeatmapIdentifierRegistry.create() } |
There was a problem hiding this comment.
what is the need for the lazy? why we cannot simply provide it in the constructor or initialize directly?
| ), | ||
| configuration = ActionEvent.Configuration(sessionSampleRate = sampleRate) | ||
| configuration = ActionEvent.Configuration(sessionSampleRate = sampleRate), | ||
| action = ddAction |
There was a problem hiding this comment.
nit: since heatmapData is immutable reference and HeatmapActionData is a data class, we can simply drop ddAction (name is a bit confusing) and do object creations here directly:
| action = ddAction | |
| action = heatmapData?.let { ActionEvent.DdAction(...) } |
|
|
||
| if (view.isAttachedToWindow) { | ||
| resolveHeatmapIdentifier(view)?.let { identity -> | ||
| val locationInWindow = IntArray(2) |
There was a problem hiding this comment.
minor: since we expect all the UI events to happen on the main thread, can we just allocate static array and re-use it for the calls? instead of allocating a new one for each call.
although probably not a big difference since it is for the taps, they don't happen that often
| attributes: Map<String, Any?>, | ||
| heatmapData: HeatmapActionData? |
There was a problem hiding this comment.
nit: generic attributes argument should come last
| val attributes: Map<String, Any?>, | ||
| override val eventTime: Time = Time() | ||
| override val eventTime: Time = Time(), | ||
| val heatmapData: HeatmapActionData? = null |
There was a problem hiding this comment.
nit: maybe put it before generic attributes and eventTime in the arguments list?
| verify(rumMonitor.mockInstance as AdvancedRumMonitor).addActionWithHeatmap( | ||
| eq(RumActionType.TAP), | ||
| eq(""), | ||
| argThat { attributes -> |
There was a problem hiding this comment.
argThat gives just a generic description when verify fails. It is better to use argumentCaptor here and assert the argument independently.
| any(), | ||
| any() | ||
| ) | ||
| verify(rumMonitor.mockInstance as AdvancedRumMonitor, never()).addActionWithHeatmap( |
There was a problem hiding this comment.
will verifyNoMoreInteractions(rumMonitor.mockInstance) work here?
| verify(rumMonitor.mockInstance as AdvancedRumMonitor).addActionWithHeatmap( | ||
| eq(RumActionType.TAP), | ||
| eq(""), | ||
| argThat { attributes -> |
There was a problem hiding this comment.
it is better to use argument captor, same for the similar tests below
6 participants
What does this PR do?
Integrates heatmap tap data into the RUM feature. When a tap gesture is detected and the view is attached to a window, GesturesListener captures the touch coordinates, the view's dimensions, and (when available) its heatmap identifier from the
HeatmapIdentifierRegistry, and packages them into a newHeatmapActionDataobject. This data flows throughAdvancedRumMonitor.addActionWithHeatmap— an internal API that keepsHeatmapActionDataoff the publicRumMonitorinterface — intoRumActionScope, where it is serialised into thedd.actionfield of the outgoingActionEvent.RumFeaturenow implementsHeatmapIdentifierRegistryProvider, exposing its lazily-initialised registry to Session Replay viaFeatureScope.unwrap()without going through the untyped feature-context map.Motivation
Support mobile heatmaps. This is PR 3 of 4, integrates the
HeatmapIdentifierRegistry(#3202) and updated schemas (#3203) into RUM.Additional Notes
Review checklist (to be filled by reviewers)