Skip to content

Commit

Permalink
Merge pull request #515 from DataDog/xgouchet/RUMM-1151/fix_interceptors
Browse files Browse the repository at this point in the history 
RUMM-1151 Use the Local first-party host detector
  • Loading branch information
@xgouchet
xgouchet authored Mar 4, 2021
2 parents 315008f + cf6fe44 commit c7a00be
Show file tree
Hide file tree
Showing 6 changed files with 75 additions and 54 deletions.
10 changes: 1 addition & 9 deletions dd-sdk-android/src/main/kotlin/com/datadog/android/DatadogInterceptor.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import com.datadog.android.core.internal.CoreFeature
import com.datadog.android.core.internal.net.FirstPartyHostDetector
import com.datadog.android.core.internal.net.identifyRequest
import com.datadog.android.core.internal.utils.devLogger
import com.datadog.android.core.internal.utils.warnDeprecated
import com.datadog.android.rum.GlobalRum
import com.datadog.android.rum.RumAttributes
import com.datadog.android.rum.RumErrorSource
Expand Down Expand Up @@ -104,14 +103,7 @@ internal constructor(
tracedRequestListener,
CoreFeature.firstPartyHostDetector,
{ AndroidTracer.Builder().build() }
) {
warnDeprecated(
"Constructor DatadogInterceptor(List<String>, TracedRequestListener)",
"1.6.0",
"1.8.0",
"DatadogInterceptor(TracedRequestListener)"
)
}
)

/**
* Creates a [TracingInterceptor] to automatically create a trace around OkHttp [Request]s, and
Expand Down
2 changes: 1 addition & 1 deletion dd-sdk-android/src/main/kotlin/com/datadog/android/core/internal/CoreFeature.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ internal object CoreFeature {
initializeClockSync(appContext)
setupInfoProviders(appContext, consent)
setupOkHttpClient(configuration.needsClearTextHttp)
firstPartyHostDetector = FirstPartyHostDetector(configuration.firstPartyHosts)
firstPartyHostDetector.addKnownHosts(configuration.firstPartyHosts)
setupExecutors()

initialized.set(true)
Expand Down
2 changes: 1 addition & 1 deletion ...k-android/src/main/kotlin/com/datadog/android/core/internal/net/FirstPartyHostDetector.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,6 @@ internal class FirstPartyHostDetector(
}

fun addKnownHosts(hosts: List<String>) {
knownHosts = knownHosts + hosts
knownHosts = knownHosts + hosts.map { it.toLowerCase(Locale.US) }
}
}
13 changes: 3 additions & 10 deletions dd-sdk-android/src/main/kotlin/com/datadog/android/tracing/TracingInterceptor.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import com.datadog.android.core.internal.CoreFeature
import com.datadog.android.core.internal.net.FirstPartyHostDetector
import com.datadog.android.core.internal.utils.devLogger
import com.datadog.android.core.internal.utils.loggableStackTrace
import com.datadog.android.core.internal.utils.warnDeprecated
import com.datadog.android.tracing.internal.TracesFeature
import com.datadog.opentracing.DDTracer
import com.datadog.trace.api.DDTags
Expand Down Expand Up @@ -98,14 +97,7 @@ internal constructor(
CoreFeature.firstPartyHostDetector,
null,
{ AndroidTracer.Builder().build() }
) {
warnDeprecated(
"Constructor TracingInterceptor(List<String>, TracedRequestListener)",
"1.6.0",
"1.8.0",
"TracingInterceptor(TracedRequestListener)"
)
}
)

/**
* Creates a [TracingInterceptor] to automatically create a trace around OkHttp [Request]s.
Expand Down Expand Up @@ -174,7 +166,8 @@ internal constructor(

private fun isRequestTraceable(request: Request): Boolean {
val url = request.url()
return firstPartyHostDetector.isFirstPartyUrl(url)
return firstPartyHostDetector.isFirstPartyUrl(url) ||
localFirstPartyHostDetector.isFirstPartyUrl(url)
}

@Suppress("TooGenericExceptionCaught", "ThrowingInternalException")
Expand Down
52 changes: 34 additions & 18 deletions ...droid/src/test/kotlin/com/datadog/android/tracing/TracingInterceptorNotSendingSpanTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,9 @@ internal open class TracingInterceptorNotSendingSpanTest {
@StringForgery
lateinit var fakeOrigin: String

@StringForgery(regex = TracingInterceptorTest.HOSTNAME_PATTERN)
lateinit var fakeLocalHosts: List<String>

// endregion

@BeforeEach
Expand All @@ -172,8 +175,8 @@ internal open class TracingInterceptorNotSendingSpanTest {
val mediaType = forge.anElementFrom("application", "image", "text", "model") +
"/" + forge.anAlphabeticalString()
fakeMediaType = MediaType.parse(mediaType)
fakeRequest = forgeRequest(forge, false)
val tracedHosts = listOf(fakeHostIp, fakeHostName)
fakeUrl = forgeUrl(forge)
fakeRequest = forgeRequest(forge)
TracesFeature.initialize(
mockAppContext,
fakeConfig
Expand All @@ -182,7 +185,7 @@ internal open class TracingInterceptorNotSendingSpanTest {
doAnswer { true }.whenever(mockDetector).isFirstPartyUrl(HttpUrl.get(fakeUrl))

GlobalTracer.registerIfAbsent(mockTracer)
testedInterceptor = instantiateTestedInterceptor(tracedHosts) { mockLocalTracer }
testedInterceptor = instantiateTestedInterceptor(fakeLocalHosts) { mockLocalTracer }
}

@AfterEach
Expand Down Expand Up @@ -215,11 +218,12 @@ internal open class TracingInterceptorNotSendingSpanTest {
}

@Test
fun `𝕄 inject tracing header 𝕎 intercept() for any request`(
fun `𝕄 inject tracing header 𝕎 intercept() {global known host}`(
@StringForgery key: String,
@StringForgery(type = StringForgeryType.ALPHA_NUMERICAL) value: String,
@IntForgery(min = 200, max = 600) statusCode: Int
) {
whenever(mockDetector.isFirstPartyUrl(HttpUrl.get(fakeUrl))).thenReturn(true)
stubChain(mockChain, statusCode)
doAnswer { invocation ->
val carrier = invocation.arguments[2] as TextMapInject
Expand All @@ -236,13 +240,15 @@ internal open class TracingInterceptorNotSendingSpanTest {
}

@Test
fun `𝕄 inject tracing header 𝕎 intercept() for any request with legacy host regex`(
fun `𝕄 inject tracing header 𝕎 intercept() {local known host}`(
@StringForgery key: String,
@StringForgery(type = StringForgeryType.ALPHA_NUMERICAL) value: String,
@IntForgery(min = 200, max = 600) statusCode: Int,
forge: Forge
) {
fakeRequest = forgeRequest(forge, true)
fakeUrl = forgeUrl(forge, forge.anElementFrom(fakeLocalHosts))
fakeRequest = forgeRequest(forge)
whenever(mockDetector.isFirstPartyUrl(HttpUrl.get(fakeUrl))).thenReturn(false)
stubChain(mockChain, statusCode)
doAnswer { invocation ->
val carrier = invocation.arguments[2] as TextMapInject
Expand Down Expand Up @@ -270,6 +276,7 @@ internal open class TracingInterceptorNotSendingSpanTest {
whenever(parentSpan.context()) doReturn parentSpanContext
whenever(mockSpanBuilder.asChildOf(parentSpanContext)) doReturn mockSpanBuilder
fakeRequest = forgeRequest(forge) { it.tag(Span::class.java, parentSpan) }
whenever(mockDetector.isFirstPartyUrl(HttpUrl.get(fakeUrl))).thenReturn(true)
doAnswer { true }.whenever(mockDetector).isFirstPartyUrl(HttpUrl.get(fakeUrl))
stubChain(mockChain, statusCode)
doAnswer { invocation ->
Expand Down Expand Up @@ -300,6 +307,7 @@ internal open class TracingInterceptorNotSendingSpanTest {
val parentSpanContext: SpanContext = mock()
whenever(mockTracer.extract<TextMapExtract>(any(), any())) doReturn parentSpanContext
whenever(mockSpanBuilder.asChildOf(any<SpanContext>())) doReturn mockSpanBuilder
whenever(mockDetector.isFirstPartyUrl(HttpUrl.get(fakeUrl))).thenReturn(true)
fakeRequest = forgeRequest(forge)
doAnswer { true }.whenever(mockDetector).isFirstPartyUrl(HttpUrl.get(fakeUrl))
stubChain(mockChain, statusCode)
Expand Down Expand Up @@ -339,6 +347,7 @@ internal open class TracingInterceptorNotSendingSpanTest {
fun `𝕄 create a span with info 𝕎 intercept() for failing request {4xx}`(
@IntForgery(min = 400, max = 500) statusCode: Int
) {
whenever(mockDetector.isFirstPartyUrl(HttpUrl.get(fakeUrl))).thenReturn(true)
stubChain(mockChain, statusCode)

val response = testedInterceptor.intercept(mockChain)
Expand All @@ -357,6 +366,7 @@ internal open class TracingInterceptorNotSendingSpanTest {
fun `𝕄 create a span with info 𝕎 intercept() for failing request {5xx}`(
@IntForgery(min = 500, max = 600) statusCode: Int
) {
whenever(mockDetector.isFirstPartyUrl(HttpUrl.get(fakeUrl))).thenReturn(true)
stubChain(mockChain, statusCode)

val response = testedInterceptor.intercept(mockChain)
Expand All @@ -373,6 +383,7 @@ internal open class TracingInterceptorNotSendingSpanTest {

@Test
fun `𝕄 create a span with info 𝕎 intercept() for 404 request`() {
whenever(mockDetector.isFirstPartyUrl(HttpUrl.get(fakeUrl))).thenReturn(true)
stubChain(mockChain, 404)

val response = testedInterceptor.intercept(mockChain)
Expand All @@ -391,6 +402,7 @@ internal open class TracingInterceptorNotSendingSpanTest {
fun `𝕄 create a span with info 𝕎 intercept() for throwing request`(
@Forgery throwable: Throwable
) {
whenever(mockDetector.isFirstPartyUrl(HttpUrl.get(fakeUrl))).thenReturn(true)
whenever(mockChain.request()) doReturn fakeRequest
whenever(mockChain.proceed(any())) doThrow throwable

Expand Down Expand Up @@ -433,6 +445,7 @@ internal open class TracingInterceptorNotSendingSpanTest {
val localSpanBuilder: DDTracer.DDSpanBuilder = mock()
val localSpan: Span = mock(extraInterfaces = arrayOf(MutableSpan::class))
GlobalTracer::class.java.setStaticValue("isRegistered", false)
whenever(mockDetector.isFirstPartyUrl(HttpUrl.get(fakeUrl))).thenReturn(true)
stubChain(mockChain, statusCode)
whenever(localSpanBuilder.asChildOf(null as SpanContext?)) doReturn localSpanBuilder
whenever(localSpanBuilder.start()) doReturn localSpan
Expand Down Expand Up @@ -463,6 +476,7 @@ internal open class TracingInterceptorNotSendingSpanTest {
val localSpanBuilder: DDTracer.DDSpanBuilder = mock()
val localSpan: Span = mock(extraInterfaces = arrayOf(MutableSpan::class))
GlobalTracer::class.java.setStaticValue("isRegistered", false)
whenever(mockDetector.isFirstPartyUrl(HttpUrl.get(fakeUrl))).thenReturn(true)
stubChain(mockChain, statusCode)
whenever(localSpanBuilder.asChildOf(null as SpanContext?)) doReturn localSpanBuilder
whenever(localSpanBuilder.start()) doReturn localSpan
Expand Down Expand Up @@ -503,6 +517,7 @@ internal open class TracingInterceptorNotSendingSpanTest {
@StringForgery tagKey: String,
@StringForgery(type = StringForgeryType.ALPHA_NUMERICAL) tagValue: String
) {
whenever(mockDetector.isFirstPartyUrl(HttpUrl.get(fakeUrl))).thenReturn(true)
stubChain(mockChain, statusCode)
whenever(
mockRequestListener.onRequestIntercepted(any(), any(), anyOrNull(), anyOrNull())
Expand All @@ -529,6 +544,7 @@ internal open class TracingInterceptorNotSendingSpanTest {
@StringForgery tagKey: String,
@StringForgery(type = StringForgeryType.ALPHA_NUMERICAL) tagValue: String
) {
whenever(mockDetector.isFirstPartyUrl(HttpUrl.get(fakeUrl))).thenReturn(true)
stubChain(mockChain, statusCode)
whenever(
mockRequestListener.onRequestIntercepted(any(), any(), anyOrNull(), anyOrNull())
Expand All @@ -555,6 +571,7 @@ internal open class TracingInterceptorNotSendingSpanTest {
@StringForgery tagKey: String,
@StringForgery(type = StringForgeryType.ALPHA_NUMERICAL) tagValue: String
) {
whenever(mockDetector.isFirstPartyUrl(HttpUrl.get(fakeUrl))).thenReturn(true)
whenever(
mockRequestListener.onRequestIntercepted(any(), any(), anyOrNull(), anyOrNull())
).doAnswer {
Expand Down Expand Up @@ -582,8 +599,7 @@ internal open class TracingInterceptorNotSendingSpanTest {
@Test
fun `M do not update the hostDetector W host list provided`(forge: Forge) {
// GIVEN
val localHosts =
forge.aList { forge.aStringMatching(TracingInterceptorTest.HOSTNAME_PATTERN) }
val localHosts = forge.aList { forge.aStringMatching(HOSTNAME_PATTERN) }

// WHEN
testedInterceptor = instantiateTestedInterceptor(localHosts) { mockLocalTracer }
Expand All @@ -597,7 +613,8 @@ internal open class TracingInterceptorNotSendingSpanTest {
@IntForgery(min = 200, max = 300) statusCode: Int,
forge: Forge
) {
fakeRequest = forgeRequest(forge, false)
fakeRequest = forgeRequest(forge)
whenever(mockDetector.isFirstPartyUrl(HttpUrl.get(fakeUrl))).thenReturn(false)
stubChain(mockChain, statusCode)

val response = testedInterceptor.intercept(mockChain)
Expand Down Expand Up @@ -629,6 +646,7 @@ internal open class TracingInterceptorNotSendingSpanTest {
) {
var called = 0
val tracedHosts = listOf(fakeHostIp, fakeHostName)
whenever(mockDetector.isFirstPartyUrl(HttpUrl.get(fakeUrl))).thenReturn(true)
testedInterceptor = instantiateTestedInterceptor(tracedHosts) {
called++
mockLocalTracer
Expand Down Expand Up @@ -661,19 +679,17 @@ internal open class TracingInterceptorNotSendingSpanTest {
whenever(chain.proceed(any())) doReturn fakeResponse
}

private fun forgeUrl(forge: Forge, knownHost: String? = null): String {
val protocol = forge.anElementFrom("http", "https")
val host = knownHost ?: forge.aStringMatching(TracingInterceptorTest.HOSTNAME_PATTERN)
val path = forge.anAlphaNumericalString()
return "$protocol://$host/$path"
}

private fun forgeRequest(
forge: Forge,
validHost: Boolean = false,
configure: (Request.Builder) -> Unit = {}
): Request {
val protocol = forge.anElementFrom("http", "https")
val host = forge.aStringMatching(TracingInterceptorTest.HOSTNAME_PATTERN)
val path = forge.anAlphaNumericalString()
fakeUrl = "$protocol://$host/$path"
if (validHost) {
whenever(mockDetector.isFirstPartyUrl(HttpUrl.get(fakeUrl))).thenReturn(true)
}

val builder = Request.Builder().url(fakeUrl)
if (forge.aBool()) {
fakeMethod = "POST"
Expand Down
Loading

0 comments on commit c7a00be

Please sign in to comment.