square / retrofit

A type-safe HTTP client for Android and the JVM
https://square.github.io/retrofit/
Apache License 2.0
43.1k stars 7.3k forks source link

Include debug info into exceptions #1162

Open artem-zinnatullin opened 9 years ago

artem-zinnatullin commented 9 years ago

What do you think about a flag for including debug info (such as request.toString()) into exceptions?

Usage:

Retrofit retrofit = new Retrofit.Builder()
    .baseUrl(API_URL)
    .addCallAdapterFactory(RxJavaCallAdapterFactory.create())
    .addConverterFactory(GsonConverterFactory.create())
    .includeDebugInfoIntoExceptions(BuildConfig.DEBUG) // only for internal builds!
    .build();

Example of a stacktrace of incorrect call via RxJava without debug info: screen shot 2015-10-03 at 23 22 48

Example of a stacktrace of incorrect call via RxJava with included debug info: screen shot 2015-10-03 at 23 23 14

Async request (RxJava, etc) loses information about its start point (jumps to the other Thread and booms there), stacktraces become useless :crying_cat_face:.

Logging is a great helper too! But sometimes you just don't want to log all the requests to keep the log readable, but you also want to see as much info as possible about the problems. Once I've spent several hours on trying to fix a request that was correct but because of a massive amount of async requests in the app — the last thing before the crash that I saw was a log of this correct request when the actual problem was in a request that fired few seconds before this.

At the moment, I have a draft of this feature where I add request.toString() to the exception message. And it works even if you fail before the request execution (converters/calladapters detection problems), also we can include more info in case of response parsing fail.

swankjesse commented 9 years ago

One problem is that URLs occasionally contain sensitive data in them, and exceptions get posted to relatively insecure bug trackers.

artem-zinnatullin commented 9 years ago

That's why it's a flag which you can enable only for internal/debug builds!

On Sun, Oct 4, 2015, 01:08 Jesse Wilson notifications@github.com wrote:

One problem is that URLs occasionally contain sensitive data in them, and exceptions get posted to relatively insecure bug trackers.

— Reply to this email directly or view it on GitHub https://github.com/square/retrofit/issues/1162#issuecomment-145294007.

@artem_zin

JakeWharton commented 9 years ago

This seems fairly low-leverage for a setting. We try and prefer sane defaults that work for most than exposing every possible configuration. I'd rather pick something that can always be used.

On Sat, Oct 3, 2015, 7:08 PM Artem Zinnatullin notifications@github.com wrote:

That's why it's a flag which you can enable only for internal/debug builds!

On Sun, Oct 4, 2015, 01:08 Jesse Wilson notifications@github.com wrote:

One problem is that URLs occasionally contain sensitive data in them, and exceptions get posted to relatively insecure bug trackers.

— Reply to this email directly or view it on GitHub https://github.com/square/retrofit/issues/1162#issuecomment-145294007.

@artem_zin

— Reply to this email directly or view it on GitHub https://github.com/square/retrofit/issues/1162#issuecomment-145296584.

artem-zinnatullin commented 9 years ago

I'd rather pick something that can always be used.

I've thought about this a lot, but I just don't know how to include URL (since it's most valuable info for the REST client) into the exception and don't leak any sensitive info… So I came to the flag that could be enabled/disabled for particular types of builds and will be disabled by default.

Moreover, I would like to use this option to include URL and response body to the exception in case of response parsing fail (imagine unexpected/incorrect JSON/etc) to make debugging and error fixing extra easy task with Retrofit!

Picasso has same setting (setIndicatorsEnabled()) and everybody loves it!

JakeWharton commented 9 years ago

Would just the static information pulled from the method be suitable? You won't get information like path/query values or headers, but it should be enough.

Picasso has same setting (setIndicatorsEnabled()) and everybody loves it!

That is a vastly different setting than anything proposed in this issue.

artem-zinnatullin commented 9 years ago

Would just the static information pulled from the method be suitable?

Hope you mean method of user created interface used for retrofit.create() and not the HTTP method? If so — it would be nice to see something like ... problem occurred during execution of GitHubService.contributors()!

JakeWharton commented 9 years ago

That and the relative path template, HTTP method, etc.

artem-zinnatullin commented 9 years ago

LGTM, I'll work on it if you don't mind.

On Fri, Oct 9, 2015, 5:26 PM Jake Wharton notifications@github.com wrote:

That and the relative path template, HTTP method, etc.

— Reply to this email directly or view it on GitHub https://github.com/square/retrofit/issues/1162#issuecomment-146886452.

@artem_zin

JakeWharton commented 9 years ago

Please do!

On Fri, Oct 9, 2015 at 11:21 AM Artem Zinnatullin notifications@github.com wrote:

LGTM, I'll work on it if you don't mind.

On Fri, Oct 9, 2015, 5:26 PM Jake Wharton notifications@github.com wrote:

That and the relative path template, HTTP method, etc.

— Reply to this email directly or view it on GitHub https://github.com/square/retrofit/issues/1162#issuecomment-146886452.

@artem_zin

— Reply to this email directly or view it on GitHub https://github.com/square/retrofit/issues/1162#issuecomment-146901715.

0xMelkor commented 6 years ago

Hi guys, how about using okhttp interceptors for logging. If you use dagger you can easily inject a Retrofit instance with logging interceptor when needed (i.e. debug mode). If you don't need logging you can inject Retrofit without It. Have a look at this https://stackoverflow.com/questions/32514410/logging-with-retrofit-2