Configure logging of HttpRequest, cleanup HttpRequest

[tobrun]

Closes #9638.

To expose the configuration of enabling/disabling logging of HttpRequest. I had to expose a utility to workaround package private implementation of HttpRequest (this class is not part of our public API). Additionally on above change, a cleanup of the class was done (remove Hungarian notation, cleanup final/static keywords for fields, class naming conventions).

[zugaldia]

Looks like we're now build the agent string with every request, could we change this so that it only happens once?

[tobrun]

afaik, a new instance of HttpRequest.java is created every time for each request. The user agent isn't retained over instances.

[zugaldia]

@tobrun Thank you for the clarification. In that case let's leave it as is for this PR but let's ticket a follow up (if it doesn't exist yet) where we want to look into avoiding that behavior. If a new instance is created for each request we aren't benefitting from reusing the connection pool from OkHttp.

[tobrun]

We are actually benefiting from the connection pool because the client is contained in a static field. Every instance of HttpRequest will have the same OkHttpClient (while this works, I would like to manage this differently, don't feel this is very clean). I will work on creating a ticket to avoid creating HttpRequests object with every request.

[zugaldia]

@tobrun You're right about the connection pool - I just included a simple Timber.d("connectionPool: %s", mClient.connectionPool().hashCode()); on the constructor and I'm getting the same hash even for different activities. However, isn't still unnecessary to recreate the user agent for every request considering it doesn't change between requests?

[tobrun]

Yeah, we can make that field static for now this would result in not recreating the user agent.
Follow up work for above in general in #9772.

[zugaldia]

Looks like HttpUrl httpUrl isn't used in this method and it could be moved into the new adaptResourceUrl

[zugaldia]

Would it be overkill to benefit from HttpUrl.newBuilder() here instead of doing it manually?

[tobrun]

This will be way cleaner:

  private String adaptResourceUrl(String resourceUrl) {
    HttpUrl httpUrl = HttpUrl.parse(resourceUrl);
    if (httpUrl != null && isMapboxHost(httpUrl)) {
      HttpUrl.Builder builder = httpUrl.newBuilder();
      builder.addQueryParameter("events", "true");
      resourceUrl = builder.build().toString();
    }
    return resourceUrl;
  }

[zugaldia]

@tobrun awesome - let's do that unless profiling indicates worse performance.

[tobrun]

@zugaldia
c23e8e3 includes correct reusing userAgent across HttpRequest instances.
PR is ready for review.

[snkashis]

How exactly does one configure this in 5.2.1? Since moving to 5.2.0 & 5.2.1, our log volume for failed map requests on poor/intermittent connections has seemed to increase dramatically. We want to disable all logging of these failed requests to avoid Crashlytics picking them up and burning battery/network to send them.

[zugaldia]

How exactly does one configure this in 5.2.1?

There's an example of how to do this in the OfflineActivity.java file included by this PR. Basically, you'd call HttpRequestUtil.setLogEnabled(true) to enable the extra logging, and HttpRequestUtil.setLogEnabled(false) to disable it.

[snkashis]

But I don't see that HttpRequestUtil file in the location mentioned in this PR anymore - https://github.com/mapbox/mapbox-gl-native/tree/master/platform/android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/http

I cannot import it currently while running
implementation 'com.mapbox.mapboxsdk:mapbox-android-sdk:5.2.1@aar'
Do I need to import something else?

[tobrun]

This PR got reverted with #9851, tracking to add this in #10672.

[snkashis]

Okay great, I tried setting this up a week ago with the same instructions provided by @zugaldia , so thought I was missing something else.