Feat: Add OkHttp client application interceptor #1330
Conversation
|
|
||
| val breadcrumb = Breadcrumb.http(request.url.toString(), request.method, code) | ||
| request.body?.contentLength().ifHasValidLength { | ||
| breadcrumb.setData("requestBodySize", it) |
There was a problem hiding this comment.
I am not sure how really valuable this information is, both for requests and responses.
There was a problem hiding this comment.
I agree, @bruno-garcia suggested it and I also found in the okhttp interceptors
https://github.com/square/okhttp/blob/master/okhttp-logging-interceptor/src/main/kotlin/okhttp3/logging/HttpLoggingInterceptor.kt
so kinda has an use case
| // TODO: collect ideas | ||
| // https://github.com/square/okhttp/blob/master/okhttp-logging-interceptor/src/main/kotlin/okhttp3/logging/HttpLoggingInterceptor.kt | ||
| // TODO: add package to options? how do we get options? does it even make sense? | ||
| // sdkVersion?.addPackage("maven:io.sentry:sentry-android-timber", BuildConfig.VERSION_NAME) |
There was a problem hiding this comment.
we can do it through Hub#getOptions but I somehow doubt it makes sense to add it for each and every instrumentation we do. I am not the one with access to reporting tools though.
There was a problem hiding this comment.
for debugging (when there are events, we know each packages were being used).
or usage of our packages to take decisions like dropping a version or stuff like this.
true, forgot about Hub#getOptions, let's add then :)
There was a problem hiding this comment.
Concerns:
- we would need to add it to all events and transactions
- since
packagesonsdkVersionsis a list (not set) creating multiple interceptors would add multiple same packages
There was a problem hiding this comment.
yeah, lets keep without it for now then and only do it when the OkHttp integration is able to captureEvent then, which would be then an integration for now it just works as an interceptor.
sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/SecondActivity.kt
Outdated
Show resolved
Hide resolved
maciejwalkowiak
changed the title
|
@marandaneto I left the TODOs you created as they are more like open questions. Other than that, interceptor is working, breadcrumbs are added, everything is tested. |
sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/SecondActivity.kt
Outdated
Show resolved
Hide resolved
| span?.finish(SpanStatus.fromHttpStatusCode(code, SpanStatus.INTERNAL_ERROR)) | ||
| } | ||
|
|
||
| val breadcrumb = Breadcrumb.http(request.url.toString(), request.method, code) |
There was a problem hiding this comment.
wondering if it makes sense to call Breadcrumb.http without code if its -1, otherwise its adding -1 which does not make sense, or Breadcrumb.http inside of it checks if its -1, or changing it to Integer and check if its null
There was a problem hiding this comment.
I changed it to always add breadcrumb even if call fails with network error.
sentry-android-okhttp/src/main/java/io/sentry/android/okhttp/SentryOkHttpInterceptor.kt
Outdated
Show resolved
Hide resolved
|
@maciejwalkowiak run |
|
I cannot approve my own PR but you have my amen |
馃摐 Description
Create spans and add breadcrumbs
A sort of guideline: getsentry/develop#254
馃挕 Motivation and Context
馃挌 How did you test it?
Unit tests, sample project.
馃摑 Checklist
馃敭 Next steps
Resolve TODOs in the code.