Add sdkIdentifier + sdkversion by electrostat · Pull Request #574 · mapbox/mapbox-java

electrostat · 2017-09-22T18:56:47Z

add parameters to initialization call
add fields to MapboxEvent
set sdkIdentifier and sdkVersion when pushing turnstile event

Description

Write here and include any issues you are fixing.

Check List

Javadoc added to any methods included
Added any test to ensure the fix works properly.

- add parameters to initialization call - add fields to MapboxEvent - set sdkIdentifier and sdkVersion when pushing turnstile event

- renaming parameters - checkstyle fix

- adjusted fallback method to preserve backwards compatability with previous versions

electrostat · 2017-09-22T21:05:24Z

What default values should be used for sdkIdentifier and sdkVersion, when using older versions prior to this change?

@ericrwolfe @mick

ericrwolfe · 2017-09-22T22:32:25Z

What default values should be used for sdkIdentifier and sdkVersion, when using older versions prior to this change?

Right now these are just null values on our server for legacy clients as our iOS Maps SDK hasn't yet been switched over to the standalone telemetry library https://github.com/mapbox/mapbox-events-ios.

I'll let @zugaldia @boundsj and @mick speak to the long term plans how we should treat these, but for now null values for the legacy Maps SDKs won't break anything.

- changed default values for earlier sdks to null for sdkIdentifier and sdkVersion - subsequent sdks will set variables

This reverts commit 03fd7c4.

- remove nulls and keep system initialized with empty strings when using old sdk (filled in on server)

Guardiola31337

We are adding sdkIdentifier and sdkVersion to the event object but not including it to the JSON object. 👀 https://github.com/mapbox/mapbox-java/blob/master/mapbox/libandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/http/TelemetryClient.java#L233-L239

Now it's working because both MapboxEvent and MapboxNavigationEvent define the same strings for keys KEY_SDK_IDENTIFIER and KEY_SDK_VERSION 👀 https://github.com/mapbox/mapbox-java/blob/master/mapbox/libandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/navigation/MapboxNavigationEvent.java#L31-L32 and https://github.com/mapbox/mapbox-java/blob/master/mapbox/libandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/http/TelemetryClient.java#L153-L154

By the way we should use the keys defined on each event because eventually they could change and mess up everything.

Guardiola31337 · 2017-09-26T09:54:44Z

...ibandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

    this.locationEngine = locationEngine;
-    initialize(context, accessToken, userAgent);
+
+    initialize(context, accessToken, userAgent, "", "");


Why not initializing sdkIdentifier and sdkVersion properties as empty and using them here?

Guardiola31337 · 2017-09-26T10:02:41Z

...ibandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

   * @param accessToken The accessToken associated with the application
   */
-  public void initialize(@NonNull Context context, @NonNull String accessToken, @NonNull String userAgent) {
+  public void initialize(@NonNull Context context, @NonNull String accessToken, @NonNull String userAgent,


We're breaking the public API here and that would mean semver. We should implement this differently to not break backwards compatibility.

Adding a new overloaded initialize method should do the trick. 👀 https://github.com/mapbox/mapbox-java/blob/master/mapbox/libandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java#L109-L120

Guardiola31337 · 2017-09-26T10:09:25Z

...ibandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

+    this.sdkVersion = sdkVersion;
+
+    if (this.context == null || TextUtils.isEmpty(this.accessToken) || TextUtils.isEmpty(this.userAgent)
+      || this.sdkIdentifier == null || this.sdkVersion == null) {


👀 #574 (comment)

for now null values for the legacy Maps SDKs won't break anything.

sdkIdentifier and sdkVersion can't be empty either. We should mark somehow the different entry points (which initialize method was called) and make these checks wisely.

Guardiola31337 · 2017-09-26T10:22:59Z

...ibandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

    event.put(MapboxEvent.KEY_CREATED, TelemetryUtils.generateCreateDate(null));
    event.put(MapboxEvent.KEY_USER_ID, mapboxVendorId);
    event.put(MapboxEvent.KEY_ENABLED_TELEMETRY, isTelemetryEnabled());
+    event.put(MapboxEvent.KEY_SDK_IDENTIFIER, sdkIdentifier);


👀 #574 (comment)

for now null values for the legacy Maps SDKs won't break anything.

We should send null values when not calling the initialize method that uses sdkIdentifier and sdkVersion explicitly. 👀 https://github.com/mapbox/mapbox-java/blob/master/mapbox/libandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/navigation/MapboxNavigationEvent.java#L284-L290

Guardiola31337 · 2017-09-26T10:23:17Z

...ibandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

    event.put(MapboxEvent.KEY_USER_ID, mapboxVendorId);
    event.put(MapboxEvent.KEY_ENABLED_TELEMETRY, isTelemetryEnabled());
+    event.put(MapboxEvent.KEY_SDK_IDENTIFIER, sdkIdentifier);
+    event.put(MapboxEvent.KEY_SDK_VERSION, sdkVersion);


Same here ☝️

- use sdkIdentifier and sdkVersion after setting them to empty strings - add sdkIdentifier and sdkVersion to JSON - added overload method -

Guardiola31337

Some minor details.

I recommend you to rebase and fix up the commits (:eyes: messages) to represent what would be merged into master for easier review 😉

BTW, great job 👍

Guardiola31337 · 2017-09-27T09:18:21Z

...ibandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

    event.put(MapboxEvent.KEY_USER_ID, mapboxVendorId);
    event.put(MapboxEvent.KEY_ENABLED_TELEMETRY, isTelemetryEnabled());

+    if (sdkIdentifier == null) {


What about simplifying this block of code (if else) extracting it into a different method and work to make the intention clearer?

We should send null values when not calling the initialize method that uses sdkIdentifier and sdkVersion explicitly (when sdkIdentifier and sdkVersion are empty). Right now, this condition is always false because we initialize sdkIdentifier and sdkVersion to "" and if any of them are null there is a previous check which will throw a TelemetryException.

Guardiola31337 · 2017-09-27T09:19:06Z

...ibandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

+      event.put(MapboxEvent.KEY_SDK_IDENTIFIER, sdkIdentifier);
+    }
+
+    if (sdkVersion == null) {


Same here ☝️

Guardiola31337 · 2017-09-27T09:26:49Z

...ibandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

+
+  public void initialize(@NonNull Context context, @NonNull String accessToken, @NonNull String userAgent,
+                         String sdkIdentifier, String sdkVersion) {
+


NIT blank line

Guardiola31337 · 2017-09-27T09:29:28Z

...ibandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

+  /**
+   * Initialize MapboxTelemetry - with sdkIdentifier + sdkVersion
+   *
+   * @param sdkIdentifier  Identifies which sdk is sending the event


Javadoc is incorrect. context, accessToken and locationEngine parameters are missing. 👀 formatting too.

Guardiola31337 · 2017-09-27T09:31:05Z

...ibandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

+
+    if (this.sdkIdentifier == null || this.sdkVersion == null) {
+      throw new TelemetryException(
+        "Please, make sure you provide a valid context, access token, and user agent. "


Exception message is not reflecting what we're checking.

Guardiola31337 · 2017-09-27T09:31:15Z

...ibandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

+    }
+
    initialize(context, accessToken, userAgent);
+


NIT blank line

Guardiola31337 · 2017-09-27T09:33:16Z

...ibandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

+   */
+
+  public void initialize(@NonNull Context context, @NonNull String accessToken, @NonNull String userAgent,
+                         String sdkIdentifier, String sdkVersion) {


We should add @NonNull annotation to sdkIdentifier and sdkVersion to reflect that those should be non null.

Guardiola31337 · 2017-09-27T09:34:31Z

...ibandroid-telemetry/src/main/java/com/mapbox/services/android/telemetry/MapboxTelemetry.java

+    if (this.context == null || TextUtils.isEmpty(this.accessToken) || TextUtils.isEmpty(this.userAgent)
+      || this.sdkIdentifier == null || this.sdkVersion == null) {
      throw new TelemetryException(
        "Please, make sure you provide a valid context, access token, and user agent. "


Exception message is not reflecting what we're checking.

- clean up code - change how null checks work for sdkIdentifier + sdkVersion

- small checkstyle fix

- useragent javadoc - clean up adding sdkIdentifier and sdkVersion to event

- changed order of useragnet within javadoc

Guardiola31337

Well done @electrostat

Add sdkIdentifier + sdkversion

7581fa8

- add parameters to initialization call - add fields to MapboxEvent - set sdkIdentifier and sdkVersion when pushing turnstile event

electrostat self-assigned this Sep 22, 2017

Anthony Agby added 2 commits September 22, 2017 15:54

Tweaks + checkstyle

a17ef8a

- renaming parameters - checkstyle fix

Fall back method

03fd7c4

- adjusted fallback method to preserve backwards compatability with previous versions

zugaldia requested a review from Guardiola31337 September 23, 2017 00:53

Anthony Agby added 3 commits September 25, 2017 10:47

Default to Null

dd4fec5

- changed default values for earlier sdks to null for sdkIdentifier and sdkVersion - subsequent sdks will set variables

Revert "Fall back method"

56b533d

This reverts commit 03fd7c4.

Remove Null

2e0e081

- remove nulls and keep system initialized with empty strings when using old sdk (filled in on server)

electrostat requested review from Guardiola31337 and removed request for Guardiola31337 September 25, 2017 17:27

electrostat added the Ready For Review label Sep 25, 2017

Guardiola31337 suggested changes Sep 26, 2017

View reviewed changes

Review 1 Changes 1

901a90c

- use sdkIdentifier and sdkVersion after setting them to empty strings - add sdkIdentifier and sdkVersion to JSON - added overload method -

electrostat requested a review from Guardiola31337 September 26, 2017 15:23

Guardiola31337 suggested changes Sep 27, 2017

View reviewed changes

Anthony Agby added 2 commits September 27, 2017 11:49

Clean-up and Null Checks

511a73c

- clean up code - change how null checks work for sdkIdentifier + sdkVersion

checkstyle

c5b938d

- small checkstyle fix

electrostat requested a review from Guardiola31337 September 27, 2017 17:06

Anthony Agby added 2 commits September 27, 2017 14:17

javadoc and adding to event

608e2d3

- useragent javadoc - clean up adding sdkIdentifier and sdkVersion to event

javadoc order

0d6bd76

- changed order of useragnet within javadoc

cammace approved these changes Sep 27, 2017

View reviewed changes

electrostat removed the request for review from Guardiola31337 September 27, 2017 20:11

Guardiola31337 approved these changes Sep 27, 2017

View reviewed changes

electrostat merged commit f1cbd9b into master Sep 27, 2017

Guardiola31337 deleted the aa-sdkIdentifier-sdkVersion-telemetry-update branch September 29, 2017 12:28


		public void initialize(@NonNull Context context, @NonNull String accessToken, @NonNull String userAgent,
		String sdkIdentifier, String sdkVersion) {

Conversation

electrostat commented Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Check List

Uh oh!

electrostat commented Sep 22, 2017

Uh oh!

ericrwolfe commented Sep 22, 2017

Uh oh!

Guardiola31337 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Guardiola31337 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Guardiola31337 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

electrostat commented Sep 22, 2017 •

edited

Loading

Guardiola31337 left a comment •

edited

Loading