Add public api to change baseUrl for telemetry #420

korshaknn · 2019-07-25T10:28:14Z

Problem
Vision team need to change telemetry url at runtime depending on a current country.

Solution
Was decided to add a public API to mapbox-events-android lib change telemetry url dynamically.

Test
Unit tests were added to check if a new url is valid and a telemetry client receives setting with a new base url.

closes 1336

codecov · 2019-07-25T10:45:00Z

Codecov Report

Merging #420 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #420      +/-   ##
============================================
+ Coverage      68.2%   68.25%   +0.04%     
- Complexity      381      382       +1     
============================================
  Files            68       68              
  Lines          2076     2079       +3     
  Branches        163      163              
============================================
+ Hits           1416     1419       +3     
  Misses          570      570              
  Partials         90       90

andrlee

@korshaknn LGTM, i highly suggest to test client with malformed url and valid url pointing to the staging environment.

Also would be nice if we'd make PR a little more descriptive for others to provide useful feedback. Link to style guide: https://github.com/mapbox/mobile-telemetry/blob/master/docs/commit-pr-style.md

I know we should probably add PR template to the repo.
cc: @alfwatt

andrlee · 2019-07-25T10:44:22Z

libtelemetry/src/test/java/com/mapbox/android/telemetry/TelemetryClientTest.java

@@ -257,6 +257,20 @@ public void checksDebugLoggingEnabledAttachment() throws Exception {
      .debug(eq("TelemetryClient"), contains(" with 1 event(s) (user agent: anyUserAgent) with payload:"));
  }

+  @Test


i wonder what happens if we pass malformed url? should we handle and implement a unit test for it?

I can add a regex to check if entered url is valid. Will it be ok?

andrlee · 2019-07-25T10:57:29Z

libtelemetry/src/main/java/com/mapbox/android/telemetry/TelemetryClientSettings.java

-  private static final String COM_EVENTS_HOST = "events.mapbox.com";
-  private static final String CHINA_EVENTS_HOST = "events.mapbox.cn";
+  @SuppressWarnings("WeakerAccess")
+  public static final String DEFAULT_STAGING_EVENTS_HOST = "api-events-staging.tilestream.net";


i suggest not to expose constants of the package private class publicly, https://github.com/mapbox/mapbox-events-android/blob/master/libtelemetry/src/main/java/com/mapbox/android/telemetry/MapboxTelemetryConstants.java is a better home for them.

andrlee · 2019-07-25T11:00:18Z

libtelemetry/src/main/java/com/mapbox/android/telemetry/TelemetryClientSettings.java

@@ -67,6 +70,10 @@ HttpUrl getBaseUrl() {
    return baseUrl;
  }

+  void setBaseUrl(String eventsHost) {


is there a way we could achieve the same with the builder? it feels like this breaks immutability of the TelemetryClientSettings class.

andrlee · 2019-07-25T11:02:15Z

libtelemetry/src/full/java/com/mapbox/android/telemetry/MapboxTelemetry.java

@@ -476,4 +476,13 @@ public Thread newThread(Runnable runnable) {
      };
    }
  }
+
+  @SuppressWarnings("WeakerAccess")


i wonder why are we suppressing this lint warning?

lint says that access can be package-private. But we need to call it from outside, so make it public and suppress warning

now since this is a new public api, i've few questions:

what happens if this method is called from the main thread?

what happens if this method is called while the network request is in flight?

what happens if this method is called from multiple threads at the same time?

I think nothing happens if we call it from the main thread.
It won't affect if the request in flight, it will affect the next request.
It's not atomic, so logic could be broken if method is called from multiple threads, but maybe it's an edge case and there is no need to handle it?

or make the method synchronized?
looks like other teams don't need that method, so it will be used only by vision team.

ok, let's make it at the very east synchronized

andrlee · 2019-07-25T11:04:19Z

libtelemetry/src/main/java/com/mapbox/android/telemetry/TelemetryClient.java

@@ -170,4 +170,8 @@ private RequestBody reverseMultiForm(MultipartBody.Builder builder) {

    return builder.build();
  }
+
+  void setBaseUrl(String eventsHost) {


i suggest to look into how this can be accomplished with the builder.

korshaknn · 2019-07-25T13:11:14Z

@andrlee ptal

yunikkk

Ok from my side, please update field as synchronized as discussed

andrlee

LGTM! A minor nitpick

andrlee · 2019-07-31T14:01:45Z

libtelemetry/src/full/java/com/mapbox/android/telemetry/MapboxTelemetry.java

+    }
+  }
+
+  private boolean isValidUrl(String eventsHost) {


should this method be static?

it is used in a single non-static method, so no need to make it static.

korshaknn requested review from andrlee, yunikkk, Chaoba and RingerJK July 25, 2019 10:28

korshaknn self-assigned this Jul 25, 2019

korshaknn added the enhancement label Jul 25, 2019

korshaknn force-pushed the knn-change-url-public-api branch from da289c9 to 3a7a151 Compare July 25, 2019 10:32

andrlee suggested changes Jul 25, 2019

View reviewed changes

korshaknn force-pushed the knn-change-url-public-api branch from 3a7a151 to 0c531c7 Compare July 25, 2019 13:10

yunikkk reviewed Jul 25, 2019

View reviewed changes

korshaknn force-pushed the knn-change-url-public-api branch from 0c531c7 to 4ff0a3f Compare July 26, 2019 07:07

andrlee approved these changes Jul 31, 2019

View reviewed changes

add public api to change baseUrl for telemetry

ca56cb5

korshaknn force-pushed the knn-change-url-public-api branch from 4ff0a3f to ca56cb5 Compare August 1, 2019 07:53

korshaknn merged commit 64f8f94 into master Aug 1, 2019

korshaknn deleted the knn-change-url-public-api branch August 1, 2019 08:22

korshaknn mentioned this pull request Aug 7, 2019

🔨 API to change baseUrl for every Environment at runtime #418

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add public api to change baseUrl for telemetry #420

Add public api to change baseUrl for telemetry #420

korshaknn commented Jul 25, 2019 •

edited

Loading

codecov bot commented Jul 25, 2019 •

edited

Loading

andrlee left a comment •

edited

Loading

andrlee Jul 25, 2019

korshaknn Jul 25, 2019

andrlee Jul 25, 2019

andrlee Jul 25, 2019

andrlee Jul 25, 2019

korshaknn Jul 25, 2019

andrlee Jul 25, 2019

korshaknn Jul 25, 2019

andrlee Jul 25, 2019

andrlee Jul 25, 2019

korshaknn commented Jul 25, 2019

yunikkk left a comment

andrlee left a comment

andrlee Jul 31, 2019

korshaknn Jul 31, 2019

Add public api to change baseUrl for telemetry #420

Add public api to change baseUrl for telemetry #420

Conversation

korshaknn commented Jul 25, 2019 • edited Loading

codecov bot commented Jul 25, 2019 • edited Loading

Codecov Report

andrlee left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

korshaknn commented Jul 25, 2019

yunikkk left a comment

Choose a reason for hiding this comment

andrlee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

korshaknn commented Jul 25, 2019 •

edited

Loading

codecov bot commented Jul 25, 2019 •

edited

Loading

andrlee left a comment •

edited

Loading