Added metadata to NavigationPerformanceEvent #1820

devotaaabel · 2019-03-15T22:29:15Z

Title

Added metadata to NavigationPerformanceEvent

What's the goal?

Goal is to add metadata to NavigationPerformanceEvent

How is it being implemented?

There are now two constructors for the NavigationPerformanceEvent, one for creating an event with metadata, and one for creating an event without metadata.

How has this been tested?

Check the events in the log
Check the events in mode

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
New and existing unit tests pass locally with my changes

codecov-io · 2019-03-20T16:01:53Z

Codecov Report

Merging #1820 into master will decrease coverage by 0.13%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #1820      +/-   ##
============================================
- Coverage     31.96%   31.82%   -0.14%     
+ Complexity      949      947       -2     
============================================
  Files           242      244       +2     
  Lines          8506     8533      +27     
  Branches        647      646       -1     
============================================
- Hits           2719     2716       -3     
- Misses         5540     5570      +30     
  Partials        247      247

Guardiola31337 · 2019-03-20T19:00:40Z

...igation/src/main/java/com/mapbox/services/android/navigation/v5/navigation/BatteryEvent.java

@@ -10,8 +11,9 @@
  private static final String IS_PLUGGED_IN_KEY = "is_plugged_in";
  private static final String BATTERY_EVENT_NAME = "battery_event";

-  BatteryEvent(String sessionId, float batteryPercentage, boolean isPluggedIn) {
-    super(sessionId, BATTERY_EVENT_NAME);
+  BatteryEvent(Context context, String sessionId, float batteryPercentage, boolean isPluggedIn,


Is the Context necessary here?

Guardiola31337 · 2019-03-20T19:06:00Z

...in/java/com/mapbox/services/android/navigation/v5/navigation/NavigationPerformanceEvent.java


-  NavigationPerformanceEvent(String sessionId, String eventName) {
+  NavigationPerformanceEvent(String sessionId, String eventName,


There are now two constructors for the NavigationPerformanceEvent, one for creating an event with metadata, and one for creating an event without metadata.

Is this necessary? Wondering if we always want to capture the metadata 🤔 I guess so, right?

Guardiola31337 · 2019-03-20T19:07:40Z

...in/java/com/mapbox/services/android/navigation/v5/navigation/NavigationPerformanceEvent.java

+  }
+
+  NavigationPerformanceEvent(@Nullable Context context, String sessionId, String eventName,
+                             NavigationPerformanceMetadata metadata) {


Is this necessary? Wondering if we always want to capture the metadata 🤔 I guess so, right?

Actually, the metadata is a parameter in both constructors.

Guardiola31337 · 2019-03-20T19:08:34Z

...in/java/com/mapbox/services/android/navigation/v5/navigation/NavigationPerformanceEvent.java

    attributes.add(new Attribute(EVENT_NAME, eventName));
+    if (context != null) {


Is this necessary? Wondering if we always want to capture the screen density 🤔 I guess so, right?

Guardiola31337 · 2019-03-20T19:10:36Z

...in/java/com/mapbox/services/android/navigation/v5/navigation/NavigationPerformanceEvent.java

    attributes.add(new Attribute(EVENT_NAME, eventName));
+    if (context != null) {
+      attributes.add(new Attribute(SCREEN_DENSITY, getScreenDensity(context)));


Is the Context necessary here?

What about passing in the screen density through the constructor so the Context wouldn't be necessary?

Guardiola31337 · 2019-03-20T19:11:53Z

...java/com/mapbox/services/android/navigation/v5/navigation/NavigationPerformanceMetadata.java

+
+  NavigationPerformanceMetadata(Context context) {
+    this.version = VERSION;
+    this.screenSize = getScreenSize(context);


Is the Context necessary here?

What about passing in the screen size through the constructor so the Context wouldn't be necessary?

Guardiola31337 · 2019-03-20T19:12:09Z

...java/com/mapbox/services/android/navigation/v5/navigation/NavigationPerformanceMetadata.java

+  NavigationPerformanceMetadata(Context context) {
+    this.version = VERSION;
+    this.screenSize = getScreenSize(context);
+    this.country = getCountry(context);


Is the Context necessary here?

What about passing in the country through the constructor so the Context wouldn't be necessary?

Guardiola31337 · 2019-03-20T19:12:22Z

...java/com/mapbox/services/android/navigation/v5/navigation/NavigationPerformanceMetadata.java

+    this.device = DEVICE;
+    this.abi = ABI;
+    this.brand = BRAND;
+    this.ram = getTotalMemory(context);


Is the Context necessary here?

What about passing in the total memory through the constructor so the Context wouldn't be necessary?

Guardiola31337 · 2019-03-20T19:13:44Z

...java/com/mapbox/services/android/navigation/v5/navigation/NavigationPerformanceMetadata.java

+  private final String gpu;
+  private final String manufacturer;
+
+  NavigationPerformanceMetadata(Context context) {


Is the Context necessary here?

Shouldn't NavigationPerformanceMetadata be a DTO / POJO?

devotaaabel · 2019-03-21T14:46:05Z

Ready for re-review @Guardiola31337

Guardiola31337 · 2019-03-21T15:04:02Z

...tion/src/main/java/com/mapbox/services/android/navigation/v5/navigation/InitialGpsEvent.java

@@ -10,7 +10,7 @@
  private static final String INITIAL_GPS_EVENT_NAME = "initial_gps_event";

  InitialGpsEvent(double elapsedTime, String sessionId) {
-    super(sessionId, INITIAL_GPS_EVENT_NAME);
+    super(sessionId, INITIAL_GPS_EVENT_NAME, null);


Wondering why we aren't capturing the metadata for InitialGpsEvent 🤔 Is there any limitation?

Yes @Guardiola31337 there is a race condition with the InitialGpsEventFactory where it's initialized before NavigationTelemetry is initialized. I tried initializing it later but got an error, I don't remember what it was.

Could we add a quick TODO comment so we don't forget to include the metadata in InitialGpsEvents eventually?

Guardiola31337 · 2019-03-21T15:04:44Z

...tion/src/main/java/com/mapbox/services/android/navigation/v5/navigation/MetadataBuilder.java

+import android.os.Build;
+import android.util.DisplayMetrics;
+
+public class MetadataBuilder {


Could MetadaBuilder be package-private?

...tion/src/main/java/com/mapbox/services/android/navigation/v5/navigation/MetadataBuilder.java

Guardiola31337 · 2019-03-21T15:18:43Z

...in/java/com/mapbox/services/android/navigation/v5/navigation/NavigationPerformanceEvent.java

    attributes.add(new Attribute(EVENT_NAME, eventName));
  }

+  private String getScreenDensity(Context context) {


Noticed that we're not adding the screen density as attribute anymore, was that on purpose? If so, we should remove this method, right?

Yes it was on purpose, I don't think it's a good idea to add added complexity for an attribute we're not sure we'll even need.

… without metadata

…en density attribute

devotaaabel added the bug Defect to be fixed. label Mar 15, 2019

devotaaabel self-assigned this Mar 15, 2019

devotaaabel requested review from Guardiola31337 and danesfeder March 15, 2019 22:29

devotaaabel force-pushed the devota-performance-metadata branch from 1b41a89 to 29e4f74 Compare March 20, 2019 16:01

devotaaabel force-pushed the devota-performance-metadata branch from 29e4f74 to d285dfa Compare March 20, 2019 16:09

devotaaabel marked this pull request as ready for review March 20, 2019 16:11

Guardiola31337 reviewed Mar 20, 2019

View reviewed changes

devotaaabel force-pushed the devota-performance-metadata branch from ac8e505 to 2916f28 Compare March 21, 2019 14:42

Guardiola31337 reviewed Mar 21, 2019

View reviewed changes

devotaaabel force-pushed the devota-performance-metadata branch from 2916f28 to d4489ce Compare March 21, 2019 15:40

Guardiola31337 approved these changes Mar 21, 2019

View reviewed changes

Devota Aabel added 5 commits March 21, 2019 15:26

Added metadata to NavigationPerformanceEvent

1502a4d

Moved display density into attributes list

59dae59

Added constructor to NavigationPerformanceEvent to allow construction…

f112ca0

… without metadata

Updated to make NavigationPerformanceMetadata a POJO and removed scre…

9f8c6bd

…en density attribute

Added todo

5a946de

devotaaabel force-pushed the devota-performance-metadata branch from 31e14d8 to 5a946de Compare March 21, 2019 19:26

devotaaabel merged commit f66f26b into master Mar 21, 2019

devotaaabel deleted the devota-performance-metadata branch March 21, 2019 19:46

danesfeder mentioned this pull request Mar 22, 2019

Release v0.33.2 #1837

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added metadata to NavigationPerformanceEvent #1820

Added metadata to NavigationPerformanceEvent #1820

devotaaabel commented Mar 15, 2019 •

edited

codecov-io commented Mar 20, 2019 •

edited

Guardiola31337 Mar 20, 2019

Guardiola31337 Mar 20, 2019

Guardiola31337 Mar 20, 2019

Guardiola31337 Mar 20, 2019

Guardiola31337 Mar 20, 2019

Guardiola31337 Mar 20, 2019

Guardiola31337 Mar 20, 2019

Guardiola31337 Mar 20, 2019

Guardiola31337 Mar 20, 2019

devotaaabel commented Mar 21, 2019

Guardiola31337 Mar 21, 2019

devotaaabel Mar 21, 2019

Guardiola31337 Mar 21, 2019

Guardiola31337 Mar 21, 2019

Guardiola31337 Mar 21, 2019

devotaaabel Mar 21, 2019


		NavigationPerformanceEvent(String sessionId, String eventName) {
		NavigationPerformanceEvent(String sessionId, String eventName,

		attributes.add(new Attribute(EVENT_NAME, eventName));
		if (context != null) {

Added metadata to NavigationPerformanceEvent #1820

Added metadata to NavigationPerformanceEvent #1820

Conversation

devotaaabel commented Mar 15, 2019 • edited

Title

What's the goal?

How is it being implemented?

How has this been tested?

Checklist

codecov-io commented Mar 20, 2019 • edited

Codecov Report

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

devotaaabel commented Mar 21, 2019

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

devotaaabel commented Mar 15, 2019 •

edited

codecov-io commented Mar 20, 2019 •

edited