-
Notifications
You must be signed in to change notification settings - Fork 71
RUM-11196: Support apollo graphql #2845
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I briefly reviewed the RUM module part of it and left some suggestions.
It is good to see finally GraphQL support in the SDK!
features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/RumConfiguration.kt
Outdated
Show resolved
Hide resolved
features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/RumFeature.kt
Outdated
Show resolved
Hide resolved
...ndroid-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumResourceScope.kt
Outdated
Show resolved
Hide resolved
...ndroid-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumResourceScope.kt
Outdated
Show resolved
Hide resolved
features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/RumAttributes.kt
Outdated
Show resolved
Hide resolved
...s/dd-sdk-android-apollo/src/main/java/com/datadog/android/apollo/DatadogApolloInterceptor.kt
Outdated
Show resolved
Hide resolved
19f82f8
to
e52dcb3
Compare
features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/RumAttributes.kt
Outdated
Show resolved
Hide resolved
features/dd-sdk-android-rum/src/main/kotlin/com/datadog/android/rum/internal/RumFeature.kt
Outdated
Show resolved
Hide resolved
...ndroid-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumResourceScope.kt
Outdated
Show resolved
Hide resolved
...ndroid-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumResourceScope.kt
Outdated
Show resolved
Hide resolved
...s/dd-sdk-android-apollo/src/main/java/com/datadog/android/apollo/DatadogApolloInterceptor.kt
Outdated
Show resolved
Hide resolved
ba2a0f6
to
287327c
Compare
287327c
to
e19f43a
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #2845 +/- ##
===========================================
- Coverage 70.81% 70.61% -0.20%
===========================================
Files 809 812 +3
Lines 29482 29582 +100
Branches 4940 5075 +135
===========================================
+ Hits 20875 20887 +12
- Misses 7246 7267 +21
- Partials 1361 1428 +67
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks alright! I've added a few comments and suggestions.
...ndroid-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumResourceScope.kt
Outdated
Show resolved
Hide resolved
...ndroid-rum/src/main/kotlin/com/datadog/android/rum/internal/domain/scope/RumResourceScope.kt
Outdated
Show resolved
Hide resolved
...id-rum/src/test/kotlin/com/datadog/android/rum/internal/domain/scope/RumResourceScopeTest.kt
Outdated
Show resolved
Hide resolved
...id-rum/src/test/kotlin/com/datadog/android/rum/internal/domain/scope/RumResourceScopeTest.kt
Outdated
Show resolved
Hide resolved
...s/dd-sdk-android-apollo/src/main/java/com/datadog/android/apollo/DatadogApolloInterceptor.kt
Outdated
Show resolved
Hide resolved
...-sdk-android-apollo/src/test/java/com/datadog/android/apollo/DatadogApolloInterceptorTest.kt
Outdated
Show resolved
Hide resolved
...-sdk-android-apollo/src/test/java/com/datadog/android/apollo/DatadogApolloInterceptorTest.kt
Outdated
Show resolved
Hide resolved
...tions/dd-sdk-android-okhttp/src/main/kotlin/com/datadog/android/okhttp/DatadogInterceptor.kt
Outdated
Show resolved
Hide resolved
private fun chainWithoutDDHeaders( | ||
internalLogger: InternalLogger, | ||
originalChain: Interceptor.Chain | ||
): Interceptor.Chain { | ||
return if (hasGraphQLHeaders(originalChain.request().headers)) { | ||
try { | ||
object : Interceptor.Chain by originalChain { | ||
override fun proceed(request: Request): Response { | ||
val cleanedRequest = request.newBuilder().apply { | ||
removeGraphQLHeaders(this) | ||
}.build() | ||
return originalChain.proceed(cleanedRequest) | ||
} | ||
} | ||
} catch (e: IllegalStateException) { | ||
internalLogger.log( | ||
level = InternalLogger.Level.WARN, | ||
target = InternalLogger.Target.MAINTAINER, | ||
messageBuilder = { ERROR_FAILED_BUILD_REQUEST }, | ||
throwable = e | ||
) | ||
originalChain // fallback to the original request | ||
} catch (e: IOException) { | ||
internalLogger.log( | ||
level = InternalLogger.Level.WARN, | ||
target = InternalLogger.Target.MAINTAINER, | ||
messageBuilder = { ERROR_FAILED_BUILD_REQUEST }, | ||
throwable = e | ||
) | ||
originalChain // fallback to the original request | ||
} | ||
} else { | ||
originalChain | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to add such complexity by building the chain. What you can do here is just remove headers before .proceed()
call and move them under the Request.tag
and take them out from there when response arrives.
By doing this they won't go out and you still have access to the necessary information.
b01d4a1
to
d58a8b3
Compare
d58a8b3
to
9463462
Compare
What does this PR do?
Adds support for Apollo GraphQL requests, which is essentially native support for graphQL (until now only available from cross-platform).
Caveats:
Support only for V4 of Apollo-Kotlin. If there will be enough demand for V3 we may extend support there.
How does it work:
ApolloInterceptor
plugin under a new integration module:dd-sdk-android-apollo
operationName
,operationType
,variables
andpayload
.DatadogInterceptor
to catch these headers and send them as part ofRumResourceScope
. After extracting the data from these headers they are removed so they won't be sent onwards to the host.DatadogApolloInterceptor(captureGraphQLPayloads = true)
Motivation
Add GraphQL support to native.
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)