Skip to content
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

Add integration for Apollo-Kotlin 3 #2109

Merged
merged 22 commits into from
Jul 19, 2022
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
770b0c9
add Interceptors for apollo3, add issue templates, adapt original tes…
lbloder Jun 7, 2022
d2a6428
Merge branch 'main' into feat/apollo-v3
lbloder Jun 7, 2022
75f9086
Merge branch 'main' into feat/apollo-v3
lbloder Jun 16, 2022
2418613
add http interceptor for apollo3, extend ISerializer to allow for des…
lbloder Jun 16, 2022
e70c24a
fix how response body size is calculated, refactor and simplify inter…
lbloder Jun 19, 2022
a99e873
Merge branch 'main' into feat/apollo-v3
lbloder Jun 19, 2022
71dabc0
update changelog
lbloder Jun 19, 2022
ea95a8c
use throwable, add constructors for java compatibility, use new apoll…
lbloder Jun 21, 2022
e32dae4
Merge branch 'main' into feat/apollo-v3
lbloder Jun 21, 2022
b2112d7
Merge branch 'main' into feat/apollo-v3
lbloder Jun 24, 2022
683d374
add custom request composer to capture more info about the apollo call
lbloder Jun 27, 2022
6f2d6be
remove unnecessary testcases in SentryApollo3InterceptorWithComposeTe…
lbloder Jun 30, 2022
ab17056
fix changelog after merge
lbloder Jun 30, 2022
700504e
Merge branch 'main' into feat/apollo-v3
lbloder Jun 30, 2022
6164c80
rollback changes to deserializer, move away from custom request compo…
lbloder Jul 3, 2022
91a0b2d
Merge branch 'main' into feat/apollo-v3
lbloder Jul 3, 2022
d58842a
update tests to use TraceSamplingDecision
lbloder Jul 3, 2022
1eba47c
add secondary extension for java compatibility, add apollo to craft.yml
lbloder Jul 11, 2022
c750672
Merge branch 'main' into feat/apollo-v3
lbloder Jul 11, 2022
5f906ab
update api
lbloder Jul 11, 2022
7d95756
Merge branch 'main' into feat/apollo-v3
lbloder Jul 13, 2022
65fdef8
Merge branch 'main' into feat/apollo-v3
adinauer Jul 19, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/ISSUE_TEMPLATE/bug_report_android.yml
Expand Up @@ -14,6 +14,7 @@ body:
- sentry-android-timber
- sentry-android-fragment
- sentry-apollo
- sentry-apollo-3
- other
validations:
required: true
Expand Down
1 change: 1 addition & 0 deletions .github/ISSUE_TEMPLATE/bug_report_java.yml
Expand Up @@ -12,6 +12,7 @@ body:
- sentry-jul
- sentry-jdbc
- sentry-apollo
- sentry-apollo-3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This requires adding the package in here https://github.com/getsentry/sentry-release-registry (for GA at least)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened getsentry/sentry-release-registry#78

Needs changes to craft.yml still

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marandaneto should I wait with adapting the craft.yml until the release registry PR is approved?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd comment it out, for now, getsentry/sentry-release-registry#78 isn't done yet and 6.2.0 is already outdated.

- sentry-kotlin-extensions
- sentry-servlet
- sentry-servlet-jakarta
Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.md
Expand Up @@ -21,6 +21,8 @@

## 6.1.1

- Add integration for Apollo-Kotlin 3 ([#2109](https://github.com/getsentry/sentry-java/pull/2109))

### Features

- Replace `tracestate` header with `baggage` header ([#2078](https://github.com/getsentry/sentry-java/pull/2078))
Expand Down
4 changes: 3 additions & 1 deletion buildSrc/src/main/java/Config.kt
Expand Up @@ -104,6 +104,8 @@ object Config {
val graphQlJava = "com.graphql-java:graphql-java:17.3"

val kotlinReflect = "org.jetbrains.kotlin:kotlin-reflect"

val apolloKotlin = "com.apollographql.apollo3:apollo-runtime:3.3.0"
}

object AnnotationProcessors {
Expand Down Expand Up @@ -131,7 +133,7 @@ object Config {
val mockitoInline = "org.mockito:mockito-inline:4.3.1"
val awaitility = "org.awaitility:awaitility-kotlin:4.1.1"
val mockWebserver = "com.squareup.okhttp3:mockwebserver:${Libs.okHttpVersion}"
val mockWebserver3 = "com.squareup.okhttp3:mockwebserver:3.14.9"
val mockWebserver4 = "com.squareup.okhttp3:mockwebserver:4.9.3"
adinauer marked this conversation as resolved.
Show resolved Hide resolved
val jsonUnit = "net.javacrumbs.json-unit:json-unit:2.32.0"
val hsqldb = "org.hsqldb:hsqldb:2.6.1"
val javaFaker = "com.github.javafaker:javafaker:1.0.2"
Expand Down
25 changes: 25 additions & 0 deletions sentry-apollo-3/api/sentry-apollo-3.api
@@ -0,0 +1,25 @@
public final class io/sentry/apollo3/SentryApollo3HttpInterceptor : com/apollographql/apollo3/network/http/HttpInterceptor {
public static final field Companion Lio/sentry/apollo3/SentryApollo3HttpInterceptor$Companion;
public static final field SENTRY_APOLLO_3_OPERATION_NAME Ljava/lang/String;
public static final field SENTRY_APOLLO_3_OPERATION_TYPE Ljava/lang/String;
public static final field SENTRY_APOLLO_3_VARIABLES Ljava/lang/String;
public fun <init> ()V
public fun <init> (Lio/sentry/IHub;)V
public fun <init> (Lio/sentry/IHub;Lio/sentry/apollo3/SentryApollo3HttpInterceptor$BeforeSpanCallback;)V
public synthetic fun <init> (Lio/sentry/IHub;Lio/sentry/apollo3/SentryApollo3HttpInterceptor$BeforeSpanCallback;ILkotlin/jvm/internal/DefaultConstructorMarker;)V
public fun dispose ()V
public fun intercept (Lcom/apollographql/apollo3/api/http/HttpRequest;Lcom/apollographql/apollo3/network/http/HttpInterceptorChain;Lkotlin/coroutines/Continuation;)Ljava/lang/Object;
}

public abstract interface class io/sentry/apollo3/SentryApollo3HttpInterceptor$BeforeSpanCallback {
public abstract fun execute (Lio/sentry/ISpan;Lcom/apollographql/apollo3/api/http/HttpRequest;Lcom/apollographql/apollo3/api/http/HttpResponse;)Lio/sentry/ISpan;
}

public final class io/sentry/apollo3/SentryApollo3HttpInterceptor$Companion {
}

public final class io/sentry/apollo3/SentryApollo3RequestComposer : com/apollographql/apollo3/api/http/HttpRequestComposer {
public fun <init> (Ljava/lang/String;)V
public fun compose (Lcom/apollographql/apollo3/api/ApolloRequest;)Lcom/apollographql/apollo3/api/http/HttpRequest;
}

79 changes: 79 additions & 0 deletions sentry-apollo-3/build.gradle.kts
@@ -0,0 +1,79 @@
import net.ltgt.gradle.errorprone.errorprone
import org.jetbrains.kotlin.gradle.tasks.KotlinCompile

plugins {
`java-library`
kotlin("jvm")
jacoco
id(Config.QualityPlugins.errorProne)
id(Config.QualityPlugins.gradleVersions)
id(Config.BuildPlugins.buildConfig) version Config.BuildPlugins.buildConfigVersion
}

configure<JavaPluginExtension> {
sourceCompatibility = JavaVersion.VERSION_1_8
targetCompatibility = JavaVersion.VERSION_1_8
}

tasks.withType<KotlinCompile>().configureEach {
kotlinOptions.jvmTarget = JavaVersion.VERSION_1_8.toString()
kotlinOptions.languageVersion = Config.kotlinCompatibleLanguageVersion
}

dependencies {
api(projects.sentry)
api(projects.sentryKotlinExtensions)

implementation(Config.Libs.apolloKotlin)

compileOnly(Config.CompileOnly.nopen)
errorprone(Config.CompileOnly.nopenChecker)
errorprone(Config.CompileOnly.errorprone)
errorprone(Config.CompileOnly.errorProneNullAway)
compileOnly(Config.CompileOnly.jetbrainsAnnotations)

// tests
testImplementation(projects.sentryTestSupport)
testImplementation(Config.Libs.coroutinesCore)
testImplementation(kotlin(Config.kotlinStdLib))
testImplementation(Config.TestLibs.kotlinTestJunit)
testImplementation(Config.TestLibs.mockitoKotlin)
testImplementation(Config.TestLibs.mockitoInline)
testImplementation(Config.TestLibs.mockWebserver4)
}

configure<SourceSetContainer> {
test {
java.srcDir("src/test/java")
}
}

jacoco {
toolVersion = Config.QualityPlugins.Jacoco.version
}

tasks.jacocoTestReport {
reports {
xml.required.set(true)
html.required.set(false)
}
}

tasks {
jacocoTestCoverageVerification {
violationRules {
rule { limit { minimum = Config.QualityPlugins.Jacoco.minimumCoverage } }
}
}
check {
dependsOn(jacocoTestCoverageVerification)
dependsOn(jacocoTestReport)
}
}

tasks.withType<JavaCompile>().configureEach {
options.errorprone {
check("NullAway", net.ltgt.gradle.errorprone.CheckSeverity.ERROR)
option("NullAway:AnnotatedPackages", "io.sentry")
}
}
@@ -0,0 +1,177 @@
package io.sentry.apollo3

import com.apollographql.apollo3.api.http.HttpHeader
import com.apollographql.apollo3.api.http.HttpRequest
import com.apollographql.apollo3.api.http.HttpResponse
import com.apollographql.apollo3.exception.ApolloHttpException
import com.apollographql.apollo3.exception.ApolloNetworkException
import com.apollographql.apollo3.network.http.HttpInterceptor
import com.apollographql.apollo3.network.http.HttpInterceptorChain
import io.sentry.Breadcrumb
import io.sentry.Hint
import io.sentry.HubAdapter
import io.sentry.IHub
import io.sentry.ISpan
import io.sentry.SentryLevel
import io.sentry.SpanStatus
import io.sentry.TracingOrigins
import io.sentry.TypeCheckHint

class SentryApollo3HttpInterceptor @JvmOverloads constructor(private val hub: IHub = HubAdapter.getInstance(), private val beforeSpan: BeforeSpanCallback? = null) :
HttpInterceptor {

override suspend fun intercept(
request: HttpRequest,
chain: HttpInterceptorChain
): HttpResponse {
val activeSpan = hub.span
return if (activeSpan == null) {
chain.proceed(request)
} else {
val span = startChild(request, activeSpan)

val cleanedHeaders = removeSentryInternalHeaders(request.headers)

val requestBuilder = request.newBuilder().apply {
headers(cleanedHeaders)
}

if (TracingOrigins.contain(hub.options.tracingOrigins, request.url)) {
val sentryTraceHeader = span.toSentryTrace()
val baggageHeader = span.toBaggageHeader()
requestBuilder.addHeader(sentryTraceHeader.name, sentryTraceHeader.value)

baggageHeader?.let {
requestBuilder.addHeader(it.name, it.value)
}
}

val modifiedRequest = requestBuilder.build()
var httpResponse: HttpResponse? = null
var statusCode: Int? = null

try {
httpResponse = chain.proceed(modifiedRequest)
statusCode = httpResponse.statusCode
span.status = SpanStatus.fromHttpStatusCode(statusCode, SpanStatus.UNKNOWN)
return httpResponse
} catch (e: Throwable) {
when (e) {
is ApolloHttpException -> {
statusCode = e.statusCode
span.status = SpanStatus.fromHttpStatusCode(statusCode, SpanStatus.INTERNAL_ERROR)
}
is ApolloNetworkException -> span.status = SpanStatus.INTERNAL_ERROR
else -> SpanStatus.INTERNAL_ERROR
}
span.throwable = e
throw e
} finally {
finish(span, modifiedRequest, httpResponse, statusCode)
}
}
}

private fun removeSentryInternalHeaders(headers: List<HttpHeader>): List<HttpHeader> {
return headers.filterNot { it.name == SENTRY_APOLLO_3_VARIABLES || it.name == SENTRY_APOLLO_3_OPERATION_NAME || it.name == SENTRY_APOLLO_3_OPERATION_TYPE }
}

private fun startChild(request: HttpRequest, activeSpan: ISpan): ISpan {
val url = request.url
val method = request.method

val operationName = operationNameFromHeaders(request)
val operation = operationName ?: "apollo.client"
val operationType = request.valueForHeader(SENTRY_APOLLO_3_OPERATION_TYPE) ?: method
val operationId = request.valueForHeader("X-APOLLO-OPERATION-ID")
val variables = request.valueForHeader(SENTRY_APOLLO_3_VARIABLES)
val description = "$operationType ${operationName ?: url}"

return activeSpan.startChild(operation, description).apply {
operationId?.let {
setData("operationId", it)
}

variables?.let {
setData("variables", it)
}
}
}

private fun operationNameFromHeaders(request: HttpRequest): String? {
return request.valueForHeader(SENTRY_APOLLO_3_OPERATION_NAME) ?: request.valueForHeader("X-APOLLO-OPERATION-NAME")
adinauer marked this conversation as resolved.
Show resolved Hide resolved
}

private fun HttpRequest.valueForHeader(key: String) = headers.firstOrNull { it.name == key }?.value

private fun finish(span: ISpan, request: HttpRequest, response: HttpResponse? = null, statusCode: Int?) {
var newSpan: ISpan = span
if (beforeSpan != null) {
try {
newSpan = beforeSpan.execute(span, request, response)
} catch (e: Throwable) {
hub.options.logger.log(SentryLevel.ERROR, "An error occurred while executing beforeSpan on ApolloInterceptor", e)
}
}
newSpan.finish()
adinauer marked this conversation as resolved.
Show resolved Hide resolved

val breadcrumb =
Breadcrumb.http(request.url, request.method.name, statusCode)

request.body?.contentLength.ifHasValidLength { contentLength ->
breadcrumb.setData("request_body_size", contentLength)
}

val hint = Hint().also {
it.set(TypeCheckHint.APOLLO_REQUEST, request)
}

response?.let { httpResponse ->
httpResponse.headersContentLength().ifHasValidLength { contentLength ->
breadcrumb.setData("response_body_size", contentLength)
}

if (!breadcrumb.data.containsKey("response_body_size")) {
httpResponse.body?.buffer?.size?.ifHasValidLength { contentLength ->
breadcrumb.setData("response_body_size", contentLength)
}
}

hint.set(TypeCheckHint.APOLLO_RESPONSE, httpResponse)
}

hub.addBreadcrumb(breadcrumb, hint)
}

// Extensions

private fun HttpResponse.headersContentLength(): Long {
return headers.firstOrNull { it.name == "Content-Length" }?.value?.toLongOrNull() ?: -1L
}

private fun Long?.ifHasValidLength(fn: (Long) -> Unit) {
if (this != null && this != -1L) {
fn.invoke(this)
}
}

/**
* The BeforeSpan callback
*/
fun interface BeforeSpanCallback {
/**
* Mutates span before being added.
*
* @param span the span to mutate or drop
* @param request the Apollo request object
* @param response the Apollo response object
*/
fun execute(span: ISpan, request: HttpRequest, response: HttpResponse?): ISpan
}

companion object {
const val SENTRY_APOLLO_3_VARIABLES = "SENTRY-APOLLO-3-VARIABLES"
const val SENTRY_APOLLO_3_OPERATION_NAME = "SENTRY-APOLLO-3-OPERATION-NAME"
const val SENTRY_APOLLO_3_OPERATION_TYPE = "SENTRY-APOLLO-3-OPERATION-TYPE"
}
}
@@ -0,0 +1,39 @@
package io.sentry.apollo3

import com.apollographql.apollo3.api.ApolloRequest
import com.apollographql.apollo3.api.CustomScalarAdapters
import com.apollographql.apollo3.api.Mutation
import com.apollographql.apollo3.api.Operation
import com.apollographql.apollo3.api.Query
import com.apollographql.apollo3.api.Subscription
import com.apollographql.apollo3.api.http.DefaultHttpRequestComposer
import com.apollographql.apollo3.api.http.HttpRequest
import com.apollographql.apollo3.api.http.HttpRequestComposer
import com.apollographql.apollo3.api.variables

class SentryApollo3RequestComposer(url: String) : HttpRequestComposer {
private val defaultHttpRequestComposer = DefaultHttpRequestComposer(url)

override fun <D : Operation.Data> compose(apolloRequest: ApolloRequest<D>): HttpRequest {
val httpRequest = defaultHttpRequestComposer.compose(apolloRequest)
val builder = httpRequest.newBuilder()
.addHeader(SentryApollo3HttpInterceptor.SENTRY_APOLLO_3_OPERATION_TYPE, operationType(apolloRequest))
.addHeader(SentryApollo3HttpInterceptor.SENTRY_APOLLO_3_OPERATION_NAME, apolloRequest.operation.name())

apolloRequest.scalarAdapters?.let {
builder.addHeader(SentryApollo3HttpInterceptor.SENTRY_APOLLO_3_VARIABLES, apolloRequest.operation.variables(it).valueMap.toString())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this blow up if there's long variables? Do we need to truncate / skip if too long?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, good point... Don't think it would blow up because header sizes are usually limited by the server side. However, in order to keep our request the Sentry Server small we should truncate/skip.
Is that something we could/should use SentryOptions#maxRequestBodySize for?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maxRequestBodySize is about the body only, headers are added as it is, IIRC this should be guarded by defaultPii

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marandaneto Actually these headers are meant to be passed internally only in order to work around the need to deserialize.
They are added by the SentryApollo3RequestComposer and then read by the SentryApollo3HttpInterceptor and then removed. Only if people use the RequestComposer without the interceptor will the headers be sent over the wire. But then they don't get any use out of it as the Interceptor is the one to send info to our Backend.

I think Alex's question was about size limits for headers in general.

As for my idea with the maxRequestBodySize: Depending on the Apollo Operation the variables could get quite big and would therefore increase our payload sent to the Backend. Should we set a fixed sizeLimit or just send the variables as they are?

We thought about guarding with defaultPii but decided against it, correct @adinauer ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd just send the variables as they are.
We don't do any trimming on the client, the server drops if it exceeds and reports to the client_reports feature.
They can use the filtering callbacks to drop stuff in case they need to.

@lbloder using the request's header for this workaround could be problematic, in case this is a request for a 3rd party and somebody isn't using the interceptor, we can be leaking some PII data, I guess?
If there's no way around getting everything we need either with both interceptors, I'd rather go with the option that has more useful data and open an issue on the apollo3 repo with a feature request explaining our use case.
Maybe @bruno-garcia has opinions here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, are we talking about sending the Request and Headers to our Backend?

Right now, we don't send the Request or its Headers to our Backend in the Interceptors, neither are we doing this in e.g. SentryOkHttpInterceptor nor in the SentryFeignClient if I'm not mistaken. @marandaneto You said this is in our spec? Does this apply to mobile as well? Should we open issues for the other integrations as well?

If we do this, then Headers should most definitely be guarded behind PII imho.

Copy link
Collaborator Author

@lbloder lbloder Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, we don't send the Request or its Headers to our Backend in the Interceptors, neither are we doing this in e.g. SentryOkHttpInterceptor nor in the SentryFeignClient if I'm not mistaken. @marandaneto You said this is in our spec? Does this apply to mobile as well? Should we open issues for the other integrations as well?

Sorry, just found your original comment, so please ignore the above.

As for my workaround with using headers to pass data between Interceptors, I don't see a problem if our headers are leaked, except that the request is getting bigger, because the headers we add only contain info that is in the request body anyways.

In terms of sending the whole request to the server, then I would definitely do it like in the SentryRequestResolver that @adinauer linked

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are not sending headers in this PR to Sentry, no need to worry, but if we do, we should guard against defaultPii, See https://github.com/getsentry/sentry-dart/blob/d75a8c698a4942816336f3bae9a50c87fa6be0a8/dio/lib/src/dio_event_processor.dart#L117

Copy link
Collaborator Author

@lbloder lbloder Jul 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made it easier to use with an extension function that adds both interceptors. Also this should make it harder to misconfigure,

The only headers we would send, and only if the SentryApollo3HttpInterceptor isn't added, are those that contain variables operationName and operationType which are all present in the request body anyways.

I would split sending the request and its headers to a separate issue for the integrations that are not sending those yet. I.e. The Apollo Integration v2 and v3, the OkHttp Integration, the FeignClient and maybe others, will look through them and create issues for those that are not sending the request and its headers to our backend.

Wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have #2148 to track.

}

return builder.build()
}

private fun <D : Operation.Data> operationType(apolloRequest: ApolloRequest<D>) = when (apolloRequest.operation) {
is Query -> "query"
is Mutation -> "mutation"
is Subscription -> "subscription"
else -> apolloRequest.operation.javaClass.simpleName
}

private val <D : Operation.Data> ApolloRequest<D>.scalarAdapters
get() = executionContext[CustomScalarAdapters]
}