RUM-2875 Introduce the FeatureContextUpdateLitener API by mariusc83 · Pull Request #1829 · DataDog/dd-sdk-android

mariusc83 · 2024-01-17T11:16:23Z

What does this PR do?

A brief description of the change being made with this pull request.

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)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Make sure you discussed the feature or bugfix with the maintaining team in an Issue
Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

mariusc83 · 2024-01-17T11:35:18Z

 ) : FeatureScope {

    internal val initialized = AtomicBoolean(false)
+    internal val contextUpdateListeners: ConcurrentHashMap<FeatureContextUpdateListener, Any?> =


not sure if I should use a ConcurrentHashMap here or just a Set and make a copy when notifying the listeners to make sure it is concurrent

codecov-commenter · 2024-01-17T16:32:35Z

Codecov Report

Merging #1829 (0011b35) into feature/sr-web-view-support (13a7578) will increase coverage by 0.03%.
The diff coverage is 69.23%.

Additional details and impacted files

@@                       Coverage Diff                       @@
##           feature/sr-web-view-support    #1829      +/-   ##
===============================================================
+ Coverage                        83.75%   83.78%   +0.03%     
===============================================================
  Files                              472      472              
  Lines                            16598    16623      +25     
  Branches                          2501     2504       +3     
===============================================================
+ Hits                             13900    13926      +26     
- Misses                            2004     2010       +6     
+ Partials                           694      687       -7

Files	Coverage Δ
...ain/kotlin/com/datadog/android/core/DatadogCore.kt	`84.10% <90.91%> (+0.41%)`	⬆️
...in/com/datadog/android/core/NoOpInternalSdkCore.kt	`15.91% <0.00%> (-0.76%)`	⬇️
...in/com/datadog/android/core/internal/SdkFeature.kt	`89.03% <61.54%> (-2.52%)`	⬇️

... and 23 files with indirect coverage changes

0xnm · 2024-01-18T09:03:27Z

+            featureName: String,
+            updateCallback: (context: MutableMap<String, Any?>) -> Unit


better to revert, not part of the actual code change.

0xnm · 2024-01-18T09:08:59Z

+    internal val contextUpdateListeners: ConcurrentHashMap<FeatureContextUpdateListener, String> =
+        ConcurrentHashMap()


Seems like it is used like a Set, and value is actually never used. In this case it can be just Collections.newSetFromMap(ConcurrentHashMap()) and placeholder for he value is not needed.

yes, wanted to use the same thing but not available for API 21 :(

It should be available https://developer.android.com/reference/java/util/Collections#newSetFromMap(java.util.Map%3CE,java.lang.Boolean%3E)

ah yes...that's not what I tested, I tested the Collections.newKeySet which is a new method, but indeed that one would work also. Tks !!

0xnm · 2024-01-18T09:11:07Z

+    }
+
+    internal fun setContextUpdateListener(listener: FeatureContextUpdateListener) {
+        synchronized(contextUpdateListeners) {


Do we actually need ConcurrentHashMap then if each access that modifies the state of contextUpdateListeners is backed by synchronized?

yes we need it as I did not want to synchronize when we notify the listeners. When I travers them for notification another thread might remove or add a new one and it will get into ConcurrentModificationException

0xnm · 2024-01-18T09:12:05Z

+                internalLogger.log(
+                    InternalLogger.Level.WARN,
+                    InternalLogger.Target.USER,
+                    { CONTEXT_UPDATE_LISTENER_ALREADY_EXISTS.format(Locale.US, wrappedFeature.name) }


But we don't actually overwrite anything, do we? Both key and value stay the same.

yes you are right...unless you are providing a different instance of the same listener. The equals method will say true and we are replacing the previous instance with the new one.

0xnm · 2024-01-18T09:13:17Z

                updateCallback(mutableContext)
                it.setFeatureContext(featureName, mutableContext)
+                feature.notifyContextUpdated(mutableContext)
+                mutableContext


why to we need this line?

oh left over sorry

0xnm · 2024-01-18T09:17:23Z

+     * @param event the updated context
+     */
+    @AnyThread
+    fun onContextUpdate(event: Map<String, Any?>)


In this case, generally speaking, we cannot reuse the same listener object for context updates of the different features.

What if we add a feature name as a first argument? That way we could re-use the same object.

Not an ask or blocker for me, just some brainstorming.

oh...yeah, it might be better indeed. Let's do this.

0xnm

lgtm, I left some suggestions

0xnm · 2024-01-22T11:05:06Z

+                // notify all the other features
+                features.filter { it.key != featureName }
+                    .forEach { (_, feature) ->
+                        feature.notifyContextUpdated(featureName, mutableContext)


even though the type on the receiver side is Map here, maybe it is worth to make a copy as a safe-guard?

Suggested change

feature.notifyContextUpdated(featureName, mutableContext)

feature.notifyContextUpdated(featureName, mutableContext.toMap())

0xnm · 2024-01-22T11:08:31Z


    companion object {
+        internal const val CONTEXT_UPDATE_LISTENER_ALREADY_EXISTS =
+            "Feature \"%s\" already has this listener registered, overwriting it."


Still there is no overwrite, it is already there. Maybe we can drop the part about overwriting?

0xnm · 2024-01-22T11:09:20Z

+    }
+
+    @Test
+    fun `𝕄 remove context update listener 𝕎 setContextUpdateListener()`(


Suggested change

fun `𝕄 remove context update listener 𝕎 setContextUpdateListener()`(

fun `𝕄 remove context update listener 𝕎 removeContextUpdateReceiver()`(

0xnm · 2024-01-22T11:13:46Z

+    }
+
+    @Test
+    fun `M update registered listeners W notifyContextUpdateListener()`(forge: Forge) {


Suggested change

fun `M update registered listeners W notifyContextUpdateListener()`(forge: Forge) {

fun `M update registered listeners W notifyContextUpdated()`(forge: Forge) {

0xnm · 2024-01-22T11:16:04Z

+        val countDownLatch = CountDownLatch(mockListeners.size)
+        mockListeners.forEach {
+            Thread {
+                testedFeature.setContextUpdateListener(it)
+                countDownLatch.countDown()
+            }.start()
+        }
+
+        // Then
+        countDownLatch.await(5, TimeUnit.SECONDS)


Alternatively this can be written without CountDownLatch:

Suggested change

val countDownLatch = CountDownLatch(mockListeners.size)

mockListeners.forEach {

Thread {

testedFeature.setContextUpdateListener(it)

countDownLatch.countDown()

}.start()

}

// Then

countDownLatch.await(5, TimeUnit.SECONDS)

mockListeners.map {

Thread {

testedFeature.setContextUpdateListener(it)

countDownLatch.countDown()

}.apply { start() }

}.forEach { it.join(5, TimeUnit.SECONDS) }

// Then

but not a big deal

mariusc83 self-assigned this Jan 17, 2024

mariusc83 force-pushed the mconstantin/rum-2875/propagate-feature-context-update-through-msb branch from 4dd25fd to 0632729 Compare January 17, 2024 11:29

mariusc83 commented Jan 17, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-2875/propagate-feature-context-update-through-msb branch 3 times, most recently from 1dc0481 to b6e49b2 Compare January 17, 2024 16:08

mariusc83 marked this pull request as ready for review January 17, 2024 16:12

mariusc83 requested review from a team as code owners January 17, 2024 16:12

0xnm reviewed Jan 18, 2024

View reviewed changes

mariusc83 force-pushed the mconstantin/rum-2875/propagate-feature-context-update-through-msb branch 2 times, most recently from 48ecd0c to c54cb2d Compare January 18, 2024 09:49

mariusc83 requested a review from 0xnm January 18, 2024 09:49

mariusc83 force-pushed the mconstantin/rum-2875/propagate-feature-context-update-through-msb branch 3 times, most recently from 4ba06aa to 0011b35 Compare January 19, 2024 16:44

0xnm previously approved these changes Jan 22, 2024

View reviewed changes

mariusc83 dismissed 0xnm’s stale review via 6869dff January 29, 2024 08:52

mariusc83 force-pushed the mconstantin/rum-2875/propagate-feature-context-update-through-msb branch from 0011b35 to 6869dff Compare January 29, 2024 08:52

RUM-2875 Introduce the FeatureContextUpdateLitener API

474b8b0

mariusc83 force-pushed the mconstantin/rum-2875/propagate-feature-context-update-through-msb branch from 6869dff to 474b8b0 Compare January 29, 2024 09:10

jonathanmos approved these changes Jan 29, 2024

View reviewed changes

xgouchet approved these changes Jan 29, 2024

View reviewed changes

mariusc83 merged commit 2470aa7 into feature/sr-web-view-support Jan 30, 2024

mariusc83 deleted the mconstantin/rum-2875/propagate-feature-context-update-through-msb branch January 30, 2024 07:57

xgouchet added this to the 2.8.x milestone Apr 5, 2024

		featureName: String,
		updateCallback: (context: MutableMap<String, Any?>) -> Unit

		internal val contextUpdateListeners: ConcurrentHashMap<FeatureContextUpdateListener, String> =
		ConcurrentHashMap()

	feature.notifyContextUpdated(featureName, mutableContext)
	feature.notifyContextUpdated(featureName, mutableContext.toMap())

	fun `𝕄 remove context update listener 𝕎 setContextUpdateListener()`(
	fun `𝕄 remove context update listener 𝕎 removeContextUpdateReceiver()`(

	fun `M update registered listeners W notifyContextUpdateListener()`(forge: Forge) {
	fun `M update registered listeners W notifyContextUpdated()`(forge: Forge) {

Uh oh!

Conversation

mariusc83 commented Jan 17, 2024

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jan 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Jan 17, 2024 •

edited

Loading