Add integration for Apollo-Kotlin 3 #2109

lbloder · 2022-06-19T22:05:28Z

📜 Description

Add tracing support for Apollo v3.

💡 Motivation and Context

Fixes #1850

💚 How did you test it?

📝 Checklist

I reviewed the submitted code
I added tests to verify the changes
I updated the docs if needed
android apollo3 integration sentry-docs#5289
No breaking changes

🔮 Next steps

…t for apollo3

…erialization with custom Deserlializer, support batched apollo requests

…ceptor

codecov-commenter · 2022-06-19T22:21:25Z

Codecov Report

Merging #2109 (65fdef8) into main (504fce8) will decrease coverage by 0.03%.
The diff coverage is 76.41%.

@@             Coverage Diff              @@
##               main    #2109      +/-   ##
============================================
- Coverage     80.95%   80.91%   -0.04%     
- Complexity     3289     3313      +24     
============================================
  Files           233      236       +3     
  Lines         12048    12154     +106     
  Branches       1596     1615      +19     
============================================
+ Hits           9753     9834      +81     
- Misses         1712     1725      +13     
- Partials        583      595      +12

Impacted Files	Coverage Δ
...io/sentry/apollo3/sentryApolloBuilderExtensions.kt	`50.00% <50.00%> (ø)`
...java/io/sentry/apollo3/SentryApollo3Interceptor.kt	`64.28% <64.28%> (ø)`
.../io/sentry/apollo3/SentryApollo3HttpInterceptor.kt	`80.23% <80.23%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 504fce8...65fdef8. Read the comment docs.

buildSrc/src/main/java/Config.kt

marandaneto · 2022-06-20T05:38:25Z

buildSrc/src/main/java/Config.kt

@@ -104,6 +104,8 @@ object Config {
        val graphQlJava = "com.graphql-java:graphql-java:17.3"

        val kotlinReflect = "org.jetbrains.kotlin:kotlin-reflect"
+
+        val apollo3 = "com.apollographql.apollo3:apollo-runtime:3.3.0"


Should we call apolloKotlin instead?

Apollo Kotlin (formerly known as Apollo Android) is a GraphQL client that generates Kotlin and Java models from GraphQL queries.

sentry-apollo-3/src/main/java/io/sentry/apollo3/SentryApollo3HttpInterceptor.kt

marandaneto · 2022-06-20T05:47:23Z

sentry-apollo-3/src/main/java/io/sentry/apollo3/SentryApollo3HttpInterceptor.kt

+                    is ApolloNetworkException -> span.status = SpanStatus.INTERNAL_ERROR
+                    else -> SpanStatus.UNKNOWN


That looks wrong, every error would be UNKNOWN if not thrown by network issues, the finally block would be better cus there might be an error but at least we have a HTTP status code?

Will double check but, afaik we get a httpResponse for all Status Codes including 4xx and 5xx in the HttpInterceptor

sentry-apollo-3/src/main/java/io/sentry/apollo3/SentryApollo3HttpInterceptor.kt

…o Builder api, update mockserver dependency

sentry-apollo-3/src/main/java/io/sentry/apollo3/SentryApollo3HttpInterceptor.kt

marandaneto · 2022-06-22T16:42:14Z

.github/ISSUE_TEMPLATE/bug_report_java.yml

@@ -12,6 +12,7 @@ body:
        - sentry-jul
        - sentry-jdbc
        - sentry-apollo
+        - sentry-apollo-3


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

Opened getsentry/sentry-release-registry#78

Needs changes to craft.yml still

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

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

lbloder · 2022-06-27T06:52:00Z

New approach:

Add a custom RequestComposer that sets internal headers that can then be read by the interceptor to get information about the call (e.g. variables). The interceptor removes these headers afterwards.
When these headers are not present, no information about the operationType or variables are added.

Drawbacks:

Users need to add both our SentryApollo3RequestComposer by using the httpTransport builder AND the SentryApollo3HttpInterceptor to their ApolloClient configuration.

ApolloClient.builder()
    .networkTransport(
        HttpNetworkTransport.Builder()
            .httpRequestComposer(SentryApollo3RequestComposer("urll"))
            .addInterceptor(SentryApollo3HttpInterceptor())
            .build()
    ).build()

adinauer

Nicely done 👏 only a few more things, otherwise LGTM.

sentry-apollo-3/src/main/java/io/sentry/apollo3/SentryApollo3HttpInterceptor.kt

adinauer · 2022-06-28T07:40:43Z

sentry-apollo-3/src/main/java/io/sentry/apollo3/SentryApollo3RequestComposer.kt

+            .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())


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

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?

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

@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 ?

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.

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.

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

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

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?

We have #2148 to track.

sentry-apollo-3/src/test/java/io/sentry/apollo3/SentryApollo3InterceptorWithComposerTest.kt

sentry/src/main/java/io/sentry/ISerializer.java

…st, make return for beforeSpan nullable, add testcase for dropping spans, add baggage Headers to test

# Conflicts: # CHANGELOG.md # buildSrc/src/main/java/Config.kt

…ser and use interceptor, add extension function to apply both interceptors at once

bruno-garcia · 2022-07-11T17:30:34Z

fyi @marandaneto is OOO until the 20th, and @romtsn will be away for 60 days from the 20th.

@adinauer might be able to help with reviews here or get a hold of folks internally.

@philipphofmann might be able to chime in too

philipphofmann · 2022-07-12T12:03:06Z

Yes, I can help, but I'm pretty packed right now. If you need a review, please ping me on Discord, @lbloder.

# Conflicts: # .craft.yml

adinauer

LGTM thanks for doing this. Just need to wait for getsentry/sentry-release-registry#78 to be merged or comment out changes to .craft.yml.

github-actions · 2022-07-19T04:58:55Z

	Fails
🚫	Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

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

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 65fdef8

lbloder added 7 commits June 8, 2022 00:24

add Interceptors for apollo3, add issue templates, adapt original tes…

770b0c9

…t for apollo3

Merge branch 'main' into feat/apollo-v3

d2a6428

Merge branch 'main' into feat/apollo-v3

75f9086

add http interceptor for apollo3, extend ISerializer to allow for des…

2418613

…erialization with custom Deserlializer, support batched apollo requests

fix how response body size is calculated, refactor and simplify inter…

e70c24a

…ceptor

Merge branch 'main' into feat/apollo-v3

a99e873

update changelog

71dabc0