PANA-5681: Add heatmaps feature flag#3500
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## gonzalezreal/PANA-5681/session-replay-heatmaps #3500 +/- ##
===============================================================================
Coverage 72.16% 72.16%
===============================================================================
Files 970 970
Lines 35656 35660 +4
Branches 5936 5936
===============================================================================
+ Hits 25730 25734 +4
+ Misses 8300 8298 -2
- Partials 1626 1628 +2
🚀 New features to boost your workflow:
|
11ba9b6 to
2df9570
Compare
5161576 to
19c81c3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19c81c3bed
ℹ️ 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".
| internal val systemRequirementsConfiguration: SystemRequirementsConfiguration, | ||
| internal val internalCallback: SessionReplayInternalCallback | ||
| internal val internalCallback: SessionReplayInternalCallback, | ||
| internal val heatmapsEnabled: Boolean |
There was a problem hiding this comment.
Avoid changing the public data-class copy ABI
Adding this constructor property to the public data class changes the generated public copy/copy$default method descriptors (visible in the updated API dump). Any already-compiled Kotlin/Java code that calls SessionReplayConfiguration.copy(...) against the previous SDK can hit NoSuchMethodError after upgrading the runtime artifact without recompiling; keep the flag out of the data-class primary constructor or otherwise preserve the old copy ABI.
Useful? React with 👍 / 👎.
This comment has been minimized.
This comment has been minimized.
| sdkCore = sdkCore, | ||
| dynamicOptimizationEnabled = dynamicOptimizationEnabled, | ||
| rumContextProvider = rumContextProvider | ||
| rumContextProvider = if (heatmapIdentifierRegistry != null) { |
There was a problem hiding this comment.
Why rumContextProvider is being controlled by heatmapIdentifierRegistry. Looks not such clear, is there any mistake? Seems like heatmapsEnabled is false by default which means it's gonna lead to API change?
| * stable identifiers that correlate with RUM action events, powering the heatmap feature. | ||
| * Disabled by default. | ||
| */ | ||
| fun setHeatmapsEnabled(heatmapsEnabled: Boolean): Builder { |
There was a problem hiding this comment.
worth to mark it as experimental API ?
What does this PR do?
Adds a flag to enable the heatmaps feature (disabled by default) - setHeatmapsEnabled
Motivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)