Merge pull request #1291 from DataDog/nogorodnikov/prepare-release-1.17.1

Prepare release 1.17.1

Author: 0xnm

CHANGELOG.md

@@ -1,3 +1,9 @@
+# 1.17.1 / 2023-02-20
+
+* [BUGFIX] RUM: Revert: Detect device's refresh rate from vital monitor. See [#1251](https://github.com/DataDog/dd-sdk-android/pull/1251)
+* [BUGFIX] RUM: The `RumEventMapper` checks `ViewEvent`s by reference. See [#1279](https://github.com/DataDog/dd-sdk-android/pull/1279)
+* [BUGFIX] Global: Remove `okhttp3.internal` package usage. See [#1288](https://github.com/DataDog/dd-sdk-android/pull/1288)
+
 # 1.17.0 / 2023-01-30
 
 * [FEATURE] Tracing: Allow the usage of OTel headers in distributed tracing. See [#1229](https://github.com/DataDog/dd-sdk-android/pull/1229)

buildSrc/src/main/kotlin/com/datadog/gradle/config/AndroidConfig.kt

@@ -18,7 +18,7 @@ object AndroidConfig {
     const val MIN_SDK_FOR_COMPOSE = 21
     const val BUILD_TOOLS_VERSION = "33.0.0"
 
-    val VERSION = Version(1, 17, 0, Version.Type.Release)
+    val VERSION = Version(1, 17, 1, Version.Type.Release)
 }
 
 @Suppress("UnstableApiUsage")

dd-sdk-android/src/main/kotlin/com/datadog/android/core/internal/net/RequestUniqueIdentifier.kt

@@ -7,7 +7,7 @@
 package com.datadog.android.core.internal.net
 
 import okhttp3.Request
-import okhttp3.internal.Util
+import java.io.IOException
 
 /**
  * Generates an identifier to uniquely track requests.
@@ -16,11 +16,25 @@ internal fun identifyRequest(request: Request): String {
     val method = request.method()
     val url = request.url()
     val body = request.body()
-    return if (body == null || body == Util.EMPTY_REQUEST) {
+    return if (body == null) {
         "$method•$url"
     } else {
-        val contentLength = body.contentLength()
+        val contentLength = try {
+            body.contentLength()
+        } catch (@Suppress("SwallowedException") ioe: IOException) {
+            0
+        }
         val contentType = body.contentType()
-        "$method•$url•$contentLength•$contentType"
+        // TODO RUMM-3062 It is possible that if requests are say GZIPed (as an example), or maybe
+        //  streaming case (?), they all will have contentLength = -1, so if they target the same URL
+        //  they are going to have same identifier, messing up reporting.
+        //  -1 is valid return value for contentLength() call according to the docs and stands
+        //  for "unknown" case.
+        //  We need to have a more precise identification.
+        if (contentType != null || contentLength != 0L) {
+            "$method•$url•$contentLength•$contentType"
+        } else {
+            "$method•$url"
+        }
     }
 }

dd-sdk-android/src/main/kotlin/com/datadog/android/rum/internal/RumFeature.kt

@@ -97,8 +97,6 @@ internal class RumFeature(
     fun initialize(context: Context, configuration: Configuration.Feature.RUM) {
         dataWriter = createDataWriter(configuration)
 
-        appContext = context.applicationContext
-
         samplingRate = configuration.samplingRate
         telemetrySamplingRate = configuration.telemetrySamplingRate
         backgroundEventTracking = configuration.backgroundEventTracking
@@ -115,6 +113,8 @@ internal class RumFeature(
 
         registerTrackingStrategies(context)
 
+        appContext = context.applicationContext
+
         sdkCore.setEventReceiver(RUM_FEATURE_NAME, this)
 
         initialized.set(true)
@@ -235,9 +235,7 @@ internal class RumFeature(
         initializeVitalMonitor(CPUVitalReader(), cpuVitalMonitor, periodInMs)
         initializeVitalMonitor(MemoryVitalReader(), memoryVitalMonitor, periodInMs)
 
-        val vitalFrameCallback = VitalFrameCallback(appContext, frameRateVitalMonitor) {
-            initialized.get()
-        }
+        val vitalFrameCallback = VitalFrameCallback(frameRateVitalMonitor) { initialized.get() }
         try {
             Choreographer.getInstance().postFrameCallback(vitalFrameCallback)
         } catch (e: IllegalStateException) {

dd-sdk-android/src/main/kotlin/com/datadog/android/rum/internal/domain/event/RumEventMapper.kt

@@ -83,7 +83,7 @@ internal data class RumEventMapper(
         // In case the event is of type ViewEvent this cannot be null according with the interface
         // but it can happen that if used from Java code to have null values. In this case we will
         // log a warning and we will use the original event.
-        return if (event is ViewEvent && (mappedEvent == null || mappedEvent != event)) {
+        return if (event is ViewEvent && (mappedEvent == null || mappedEvent !== event)) {
             internalLogger.log(
                 InternalLogger.Level.WARN,
                 InternalLogger.Target.USER,

dd-sdk-android/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumViewScope.kt

@@ -6,8 +6,16 @@
 
 package com.datadog.android.rum.internal.domain.scope
 
+import android.annotation.SuppressLint
+import android.app.Activity
+import android.content.Context
+import android.os.Build
+import android.view.WindowManager
 import androidx.annotation.WorkerThread
+import androidx.fragment.app.Fragment
 import com.datadog.android.core.internal.net.FirstPartyHostHeaderTypeResolver
+import com.datadog.android.core.internal.system.BuildSdkVersionProvider
+import com.datadog.android.core.internal.system.DefaultBuildSdkVersionProvider
 import com.datadog.android.core.internal.utils.internalLogger
 import com.datadog.android.core.internal.utils.loggableStackTrace
 import com.datadog.android.core.internal.utils.resolveViewUrl
@@ -51,6 +59,7 @@ internal open class RumViewScope(
     internal val memoryVitalMonitor: VitalMonitor,
     internal val frameRateVitalMonitor: VitalMonitor,
     private val contextProvider: ContextProvider,
+    private val buildSdkVersionProvider: BuildSdkVersionProvider = DefaultBuildSdkVersionProvider(),
     private val viewUpdatePredicate: ViewUpdatePredicate = DefaultViewUpdatePredicate(),
     private val featuresContextResolver: FeaturesContextResolver = FeaturesContextResolver(),
     internal val type: RumViewType = RumViewType.FOREGROUND,
@@ -120,6 +129,7 @@ internal open class RumViewScope(
         }
     }
 
+    private var refreshRateScale: Double = 1.0
     private var lastFrameRateInfo: VitalInfo? = null
     private var frameRateVitalListener: VitalListener = object : VitalListener {
         override fun onVitalUpdate(info: VitalInfo) {
@@ -140,6 +150,8 @@ internal open class RumViewScope(
         cpuVitalMonitor.register(cpuVitalListener)
         memoryVitalMonitor.register(memoryVitalListener)
         frameRateVitalMonitor.register(frameRateVitalListener)
+
+        detectRefreshRateScale(key)
     }
 
     // region RumScope
@@ -662,6 +674,8 @@ internal open class RumViewScope(
         val eventJsRefreshRate = performanceMetrics[RumPerformanceMetric.JS_FRAME_TIME]
             ?.toInversePerformanceMetric()
 
+        val eventRefreshRateScale = refreshRateScale
+
         val updatedDurationNs = resolveViewDuration(event)
         val rumContext = getRumContext()
 
@@ -697,8 +711,8 @@ internal open class RumViewScope(
                         cpuTicksPerSecond = eventCpuTicks?.let { (it * ONE_SECOND_NS) / updatedDurationNs },
                         memoryAverage = memoryInfo?.meanValue,
                         memoryMax = memoryInfo?.maxValue,
-                        refreshRateAverage = refreshRateInfo?.meanValue,
-                        refreshRateMin = refreshRateInfo?.minValue,
+                        refreshRateAverage = refreshRateInfo?.meanValue?.let { it * eventRefreshRateScale },
+                        refreshRateMin = refreshRateInfo?.minValue?.let { it * eventRefreshRateScale },
                         isSlowRendered = isSlowRendered,
                         frustration = ViewEvent.Frustration(eventFrustrationCount.toLong()),
                         flutterBuildTime = eventFlutterBuildTime,
@@ -964,6 +978,29 @@ internal open class RumViewScope(
         return stopped && activeResourceScopes.isEmpty() && (pending <= 0L)
     }
 
+    /*
+     * The refresh rate needs to be computed with each view because:
+     * - it requires a context with a UI (we can't get this from the application context);
+     * - it can change between different activities (based on window configuration)
+     */
+    @SuppressLint("NewApi")
+    @Suppress("DEPRECATION")
+    private fun detectRefreshRateScale(key: Any) {
+        val activity = when (key) {
+            is Activity -> key
+            is Fragment -> key.activity
+            is android.app.Fragment -> key.activity
+            else -> null
+        } ?: return
+
+        val display = if (buildSdkVersionProvider.version() >= Build.VERSION_CODES.R) {
+            activity.display
+        } else {
+            (activity.getSystemService(Context.WINDOW_SERVICE) as? WindowManager)?.defaultDisplay
+        } ?: return
+        refreshRateScale = STANDARD_FPS / display.refreshRate
+    }
+
     enum class RumViewType {
         NONE,
         FOREGROUND,

dd-sdk-android/src/main/kotlin/com/datadog/android/rum/internal/vitals/VitalFrameCallback.kt

@@ -6,9 +6,7 @@
 
 package com.datadog.android.rum.internal.vitals
 
-import android.content.Context
 import android.view.Choreographer
-import android.view.WindowManager
 import com.datadog.android.core.internal.utils.internalLogger
 import com.datadog.android.v2.api.InternalLogger
 import java.util.concurrent.TimeUnit
@@ -17,7 +15,6 @@ import java.util.concurrent.TimeUnit
  * Reads the UI framerate based on the [Choreographer.FrameCallback] and notify a [VitalObserver].
  */
 internal class VitalFrameCallback(
-    private val appContext: Context,
     private val observer: VitalObserver,
     private val keepRunning: () -> Boolean
 ) : Choreographer.FrameCallback {
@@ -30,9 +27,7 @@ internal class VitalFrameCallback(
         if (lastFrameTimestampNs != 0L) {
             val durationNs = (frameTimeNanos - lastFrameTimestampNs).toDouble()
             if (durationNs > 0.0) {
-                val refreshRateScale = detectRefreshRateScale()
-                val rawFps = ONE_SECOND_NS / durationNs
-                val frameRate = rawFps * refreshRateScale
+                val frameRate = ONE_SECOND_NS / durationNs
                 if (frameRate in VALID_FPS_RANGE) {
                     observer.onNewSample(frameRate)
                 }
@@ -55,35 +50,13 @@ internal class VitalFrameCallback(
         }
     }
 
-    @Suppress("DEPRECATION")
-    private fun detectRefreshRateScale(): Double {
-        val windowManager = appContext.getSystemService(Context.WINDOW_SERVICE) as? WindowManager
-        return if (windowManager == null) {
-            internalLogger.log(
-                InternalLogger.Level.WARN,
-                InternalLogger.Target.MAINTAINER,
-                "WindowManager is null, can't detect max refresh rate!"
-            )
-            1.0
-        } else if (windowManager.defaultDisplay == null) {
-            internalLogger.log(
-                InternalLogger.Level.WARN,
-                InternalLogger.Target.MAINTAINER,
-                "Display is null, can't detect max refresh rate!"
-            )
-            1.0
-        } else {
-            STANDARD_FPS / windowManager.defaultDisplay.refreshRate
-        }
-    }
-
     // endregion
 
     companion object {
         val ONE_SECOND_NS: Double = TimeUnit.SECONDS.toNanos(1).toDouble()
 
         private const val MIN_FPS: Double = 1.0
-        private const val STANDARD_FPS: Double = 60.0
-        val VALID_FPS_RANGE = MIN_FPS.rangeTo(STANDARD_FPS)
+        private const val MAX_FPS: Double = 240.0
+        val VALID_FPS_RANGE = MIN_FPS.rangeTo(MAX_FPS)
     }
 }

dd-sdk-android/src/test/kotlin/com/datadog/android/core/internal/net/RequestUniqueIdentifierTest.kt

@@ -13,13 +13,17 @@ import fr.xgouchet.elmyr.junit5.ForgeExtension
 import okhttp3.MediaType
 import okhttp3.Request
 import okhttp3.RequestBody
+import okio.BufferedSink
 import org.assertj.core.api.Assertions.assertThat
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.extension.ExtendWith
 import org.junit.jupiter.api.extension.Extensions
+import org.junit.jupiter.params.ParameterizedTest
+import org.junit.jupiter.params.provider.ValueSource
 import org.mockito.junit.jupiter.MockitoExtension
 import org.mockito.junit.jupiter.MockitoSettings
 import org.mockito.quality.Strictness
+import java.io.IOException
 
 @Extensions(
     ExtendWith(MockitoExtension::class),
@@ -172,4 +176,40 @@ internal class RequestUniqueIdentifierTest {
         assertThat(id)
             .isEqualTo("DELETE•$fakeUrl•$contentLength•$fakeContentType; charset=utf-8")
     }
+
+    @ValueSource(strings = ["POST", "PUT", "PATCH", "DELETE"])
+    @ParameterizedTest
+    fun `identify request { body#contentLength throws exception }`(
+        method: String
+    ) {
+        val body = object : RequestBody() {
+            override fun contentLength(): Long {
+                throw IOException("")
+            }
+
+            override fun contentType(): MediaType? {
+                return MediaType.parse(fakeContentType)
+            }
+
+            override fun writeTo(sink: BufferedSink) {
+                // no-op
+            }
+        }
+        val request = Request.Builder()
+            .apply {
+                when (method) {
+                    "POST" -> post(body)
+                    "PUT" -> put(body)
+                    "PATCH" -> patch(body)
+                    "DELETE" -> delete(body)
+                }
+            }
+            .url(fakeUrl)
+            .build()
+
+        val id = identifyRequest(request)
+
+        assertThat(id)
+            .isEqualTo("$method•$fakeUrl•0•$fakeContentType")
+    }
 }