Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Log failed http request url #10830

Merged
merged 1 commit into from
Jan 9, 2018
Merged

Log failed http request url #10830

merged 1 commit into from
Jan 9, 2018

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Jan 4, 2018

closes #10825.

cc @danswick

@tobrun tobrun added the Android Mapbox Maps SDK for Android label Jan 4, 2018
@tobrun tobrun added this to the android-v5.3.1 milestone Jan 4, 2018
@tobrun tobrun self-assigned this Jan 4, 2018
@tobrun tobrun force-pushed the 10825-log-url-failed-request branch from cf5cb44 to 9c39198 Compare January 4, 2018 13:31
@tobrun tobrun force-pushed the 10825-log-url-failed-request branch from 9c39198 to 1a5e35a Compare January 5, 2018 13:44
Copy link
Member

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

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

Minor changes would be nice, besides that looks good!

@@ -180,6 +181,9 @@ private void onFailure(Exception e) {
// PERMANENT_ERROR
Timber.w("Request failed due to a permanent error: %s", errorMessage);
}
if (logRequestUrl) {
Timber.v(call.request().url().toString());
Copy link
Member

Choose a reason for hiding this comment

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

Can we give some more context to this log and maybe make it a Timber.w()? Alternatively, we could include it in above logs with simplified if.

@@ -217,4 +221,8 @@ private String getApplicationIdentifier() {
static void enableLog(boolean enabled) {
logEnabled = enabled;
}

static void enablePrintRequestUrlOnFaillure(boolean enabled) {
Copy link
Member

Choose a reason for hiding this comment

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

"faillure" - typo ;)

@@ -16,4 +16,20 @@
public static void setLogEnabled(boolean enabled) {
HTTPRequest.enableLog(enabled);
}

/**
* Enable printing of the request url when a error occurred. Default value is false.
Copy link
Member

Choose a reason for hiding this comment

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

"a error" - typo ;)

@tobrun tobrun force-pushed the 10825-log-url-failed-request branch 2 times, most recently from 8a55f2b to 96696ea Compare January 8, 2018 12:16
@tobrun
Copy link
Member Author

tobrun commented Jan 8, 2018

@LukasPaczos, revisited the approach taken, instead of the if/else construct, I went with the Timber.log(int severity,.. implementation instead.

@tobrun tobrun force-pushed the 10825-log-url-failed-request branch from 96696ea to 3db6bdc Compare January 9, 2018 09:45
@tobrun tobrun merged commit 610fb87 into master Jan 9, 2018
@tobrun tobrun deleted the 10825-log-url-failed-request branch January 9, 2018 16:05
@tobrun tobrun mentioned this pull request Jan 10, 2018
23 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include request URL when logging HTTP request messages
2 participants