chucker icon indicating copy to clipboard operation
chucker copied to clipboard

Consider updating docs/readme to addNetworkInterceptor() instead of addInterceptor()

Open ColtonIdle opened this issue 2 years ago • 8 comments

:warning: Is your feature request related to a problem? Please describe

I recently had an issue where I couldn't see my headers in the request tab. I use chucker as an on-device replacement for chalres proxy and so not showing okhttp headers by default (like the docs show) is sub-par imo. So I'm basically turning this comment by @MiSikora into a proper feature request. https://github.com/ChuckerTeam/chucker/issues/758#issuecomment-1007020678

:bulb: Describe the solution you'd like

Update docs from

// Don't forget to plug the ChuckerInterceptor inside the OkHttpClient
val client = OkHttpClient.Builder()
        .addInterceptor(chuckerInterceptor)
        .build()

to

// Don't forget to plug the ChuckerInterceptor inside the OkHttpClient
val client = OkHttpClient.Builder()
        .addNetworkInterceptor(chuckerInterceptor) // use network interceptor if you want to show all network traffic (like a proxy would)
        // or
        .addInterceptor(chuckerInterceptor) // use an application interceptor if you want to show... idk why you would want an app interceptor honestly. 🤷‍♀️ )
        .build()

:page_facing_up: Additional context

As per the okhttp docs... Network Interceptors "Observe the data just as it will be transmitted over the network." so to me... chucker is a library that is made to replace a proxy a proxy is used to observes data over the network why recommend/guide users towards using an application interceptor?

Anyway. I just hope some minor changes to docs will help someone make an informed decision when adding the library, vs just following the docs for adding an app interceptor.

ColtonIdle avatar Jan 07 '22 14:01 ColtonIdle

Could you make a PR for this?

why recommend/guide users towards using an application interceptor?

I don't think it is a recommendation. Just the way things were documented. I personally never use Chucker as an application interceptor, but I think there are some advantages. For example, you don't pollute calls preview with redirect responses.

MiSikora avatar Jan 07 '22 15:01 MiSikora

I didn't open a PR because I didn't know if there were some advantages to app vs network (you highlighted a good one though "not polluting chucker with redirect responses"

I can try to look up some more pros and cons to try to compare them at some point in the future.

ColtonIdle avatar Jan 07 '22 15:01 ColtonIdle

Just as a side note: there is a FAQ section in the README where this is clearly presented: https://github.com/ChuckerTeam/chucker#faq-

Why are some of my request headers (e.g. Content-Encoding or Accept-Encoding) missing? Why are retries and redirects not being captured discretely? Why are my encoded request/response bodies not appearing as plain text? Please refer to this section of the OkHttp documentation. You can choose to use Chucker as either an application or network interceptor, depending on your requirements.

Maybe we can just highlight it further?

cortinico avatar Jan 07 '22 17:01 cortinico

Yeah. To me it's less about that section, but more about the fact that the docs say this:

// Don't forget to plug the ChuckerInterceptor inside the OkHttpClient
val client = OkHttpClient.Builder()
        .addInterceptor(chuckerInterceptor)
        .build()

You gotta admit that to people that aren't familiar to interceptors that this will go right over their head. And I'd make the same argument about headers. I didn't even realize I had missing headers in the request tabs because I mostly look at the response tab, but today I had to figure out why a request wasn't working and I was surprised that my app for like over a year was missing headers in the request tab section. lol

ColtonIdle avatar Jan 10 '22 05:01 ColtonIdle

I didn't even realize I had missing headers in the request tabs because I mostly look at the response tab

When Chucker is added as an application interceptor, some of the response's headers might also be missing.

https://github.com/square/okhttp/blob/b8ebe99bb4b6b516b4a2e01ef103fc536a0cf502/okhttp/src/jvmMain/kotlin/okhttp3/internal/http/BridgeInterceptor.kt#L95-L99

https://github.com/square/okhttp/blob/b8ebe99bb4b6b516b4a2e01ef103fc536a0cf502/okhttp-brotli/src/main/kotlin/okhttp3/brotli/internal/brotli.kt#L41-L45

MiSikora avatar Jan 10 '22 10:01 MiSikora

On kotlinglang slack jakewharton answered a question of mine and said:

Network interceptor sees all, especially if it's last in the chain

With that said, I'm definitely moving to a network interceptor thats last in the chain since I want chucker to act as a complete replacement of charles proxy.

ColtonIdle avatar Jan 10 '22 21:01 ColtonIdle

I make heavy use of interceptors to do dynamic request modifications in every app I build in order to keep my retrofit services high level. Things like building the base url to use based on annotations in order to target different environments.

Looking at the okhttp docs there is one item that really stands out to me.

Observe the data just as it will be transmitted over the network.

This also means the logging interceptors need to be the last ones added so they are the last to run.

for the concern of

not polluting chucker with redirect responses

Perhaps adding an option to collapse redirects to ChuckerInterceptor.Builder would be sufficient? Chucker could tag the request so that it is easier to track what redirect came from what original request (assuming there isn't some other clear mechanism to track that)?

trevjonez avatar Jan 14 '22 16:01 trevjonez

I found out today that charles also shows redirects and so im okay with them because i want chucker to replaces charles. 😄

ColtonIdle avatar Jan 15 '22 08:01 ColtonIdle