initial commit of the new telemetry handler #2445

olegzil · 2020-01-31T19:27:25Z

Description

Telemetry implementation. MapboxNavigationTelemetry is a singleton object that transmits all telemetry events to the back end servers. The usage of this class requires that it is thread thread safe. Although it has a single public method, postTelemetryEvent() that handles all possible telemetry data to be transmitted to the server, it also registers itself with MapboxNavigation to receive various updates. These updates are asynchronous and may arrive at the same time that postTelemetryEvent() call is active. The code makes sure that the notification and the call do not cause a race condition. This is done using two channels one receive location updates the other receives onProgress events. All calls that may read or write the internal buffers of MapboxNavigationTelemetry are serialized using LocationBufferControl. It contains the commend to execute and a lambda that represents the command. The method monitorChannels() is what serializes access.

Currently only User Feedback events are handled. Each event becomes a member variable of TelemetryUserFeedbackWrapper. One of these will have to be written for each event to be sent to the back end servers. So to generate the required event data, you have to implement a TelemetryWrapper that collects the required data for this event.

Please include a brief summary of the change and which issue is fixed (e.g., fixes #issue)

I have added any issue links
I have added all related labels (bug, feature, new API(s), SEMVER, etc.)
I have added the appropriate milestone and project boards

Goal

Please describe the PR goals. Just the stuff needed to implement the fix / feature and a simple rationale. It could contain many check points if needed

Implementation

Please include all the relevant things implemented and also rationale, clarifications / disclaimers etc. related to the approach used. It could be as self code companion comments

Screenshots or Gifs

Please include all the media files to give some context about what's being implemented or fixed. It's not mandatory to upload screenshots or gifs, but for most of the cases it becomes really handy to get into the scope of the feature / bug being fixed and also it's REALLY useful for UI related PRs

Testing

Please describe the manual tests that you ran to verify your changes

I have tested locally (including SNAPSHOT upstream dependencies if needed) through testapp/demo app and run all activities to avoid regressions
I have tested via a test drive, or a simulation/mock location app
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have updated the CHANGELOG including this PR

build.gradle

libnavigation-base/src/main/java/com/mapbox/navigation/base/exceptions/NavigationException.kt

libnavigation-core/build.gradle

libnavigation-core/src/main/java/com/mapbox/navigation/core/telemetry/PhoneState.kt

Guardiola31337 · 2020-02-03T04:42:20Z

...gation-core/src/main/java/com/mapbox/navigation/core/telemetry/TelemetryEventUserFeedback.kt

+import android.location.Location
+import com.google.gson.Gson
+
+internal enum class FeedbackType(val feedbackType: String) {


This is never used.

It will be used once UserFeedback events are posted to Telemetry

👍 removed

...gation-core/src/main/java/com/mapbox/navigation/core/telemetry/TelemetryEventUserFeedback.kt

libnavigation-core/src/main/java/com/mapbox/navigation/core/telemetry/audio/AudioTypeChain.kt

...avigation-core/src/main/java/com/mapbox/navigation/core/telemetry/audio/AudioTypeResolver.kt

RingerJK

looks good 👍 need some changes

...igation-core/src/main/java/com/mapbox/navigation/core/telemetry/MapboxNavigationTelemetry.kt

RingerJK · 2020-02-03T10:02:03Z

libnavigation-core/src/main/java/com/mapbox/navigation/core/telemetry/NavigationUtils.kt

+import com.mapbox.navigation.core.telemetry.audio.AudioTypeChain
+import kotlin.math.floor
+
+internal object NavigationUtils {


also would be great have it as extensions

libnavigation-core/src/main/java/com/mapbox/navigation/core/telemetry/PhoneState.kt

...gation-core/src/main/java/com/mapbox/navigation/core/telemetry/TelemetryEventUserFeedback.kt

...igation-core/src/main/java/com/mapbox/navigation/core/telemetry/MapboxNavigationTelemetry.kt

LukasPaczos

Good progress @olegzil. Adding to all other comments, I'd love to see an initial test structure implementation. The concepts you're introducing might allow us to collect metrics from any thread (another question is, do we need it?), but it'd be great if the architecture was designed in a testable manner from the get-go.

LukasPaczos · 2020-02-03T11:42:46Z

...igation-core/src/main/java/com/mapbox/navigation/core/telemetry/MapboxNavigationTelemetry.kt

+            .build()
+
+    // Call back that receives
+    private val routeProgressListener = object : RouteProgressObserver {


I agree, and preferably it should already be parsed into whatever format the telemetry wrappers expect. This way, we're removing the cross dependency.

...igation-core/src/main/java/com/mapbox/navigation/core/telemetry/MapboxNavigationTelemetry.kt

olegzil · 2020-02-03T19:31:11Z

Good progress @olegzil. Adding to all other comments, I'd love to see an initial test structure implementation. The concepts you're introducing might allow us to collect metrics from any thread (another question is, do we need it?), but it'd be great if the architecture was designed in a testable manner from the get-go.

I am working on a set of Unit tests that should test this code extensively. Regarding the need for thread safety. Two answers regarding this subject. In this particular case the Telemetry class exposes one public method (not counting initialized()) and subscribes to two interfaces that have a combined three callbacks.
postTelemetryEvent() -- a method that can be called from any thread. This method cannot block the caller. It is used to report telemetry events from any part of the SDK, running on any thread.
The call backs are:
RouteProgressObserver and LocationEngineCallback. These two are usually called from the Main thread. Both the public method and the callbacks operate on the same data: locationBuffer and telemetryEventsQueue. As you can see, since object state can potentially be modified from multiple threads, access to the state should be serialized.

On a more general note, the SDK has multiple Singletons, each maintains a state that can be accessed via the singletons public methods. We should consider making these singletons thread safe. Finally, do we want the SDK to be reactive? Making it reactive requires that no public method blocks the caller in addition to guaranteeing thread sefety.

...igation-core/src/main/java/com/mapbox/navigation/core/telemetry/MapboxNavigationTelemetry.kt

libnavigation-core/build.gradle

libnavigation-core/src/main/java/com/mapbox/navigation/core/MapboxNavigation.kt

libnavigation-core/src/test/java/com/mapbox/navigation/core/MapboxNavigationTelemetryTest.kt

libnavigation-core/src/test/java/com/mapbox/navigation/core/ContextCreationHelpers.kt

libnavigation-core/src/test/java/com/mapbox/navigation/core/MapboxNavigationTest.kt

RingerJK · 2020-02-06T11:41:21Z

...igation-core/src/main/java/com/mapbox/navigation/core/telemetry/MapboxNavigationTelemetry.kt

+            }
+
+    private fun validateAccessToken(accessToken: String?) {
+        if (accessToken.isNullOrEmpty() ||


you might check it here

Suggested change

if (accessToken.isNullOrEmpty() ||

if (accessToken.isNullOrEmpty()

|| ( !accessToken.toLowerCase(Locale.US).startsWith("pk.")

|| !accessToken.toLowerCase(Locale.US).startsWith("sk."))

That would be incorrect. a token can start with pk or sk. If we use an "or" than if either one is false, the entire if-statement fails. We want it to fail only if neither is correct. Neither is modeled with "and". Like this:
if (accessToken.isNullOrEmpty() || ( !accessToken.toLowerCase(Locale.US).startsWith("pk.") && !accessToken.toLowerCase(Locale.US).startsWith("sk."))

yep, you're right, thanks!

...gation-core/src/main/java/com/mapbox/navigation/core/telemetry/TelemetryEventUserFeedback.kt

libnavigation-base/build.gradle

LukasPaczos · 2020-02-12T10:32:45Z

libnavigation-core/build.gradle

    implementation dependenciesList.mapboxSdkTurf
    implementation dependenciesList.mapboxAndroidAccounts
+    implementation dependenciesList.mapboxEvents
+    implementation project(':liblogger')


Let's not depend on the liblogger module. We have to remove it anyway and wait for mapbox/mapbox-base-android#21 to be released. Please use android.util.Log until then.

libnavigation-core/src/main/java/com/mapbox/navigation/core/MapboxNavigation.kt

LukasPaczos · 2020-02-12T10:35:21Z

libnavigation-core/src/main/java/com/mapbox/navigation/core/MapboxNavigationOptions.kt

+/**
+ * Immutable and can't be changed after passing into [MapboxNavigation].
+ */
+data class MapboxNavigationOptions(


Let's remove this class and use already present NavigationOptions if needed.

LukasPaczos · 2020-02-12T10:36:00Z

libnavigation-core/src/main/java/com/mapbox/navigation/core/NavigationConstants.kt

+ *
+ * @since 0.1.0
+ */
+object NavigationConstants {


Let's remove this class as well and move only the required constant into the correct file.

LukasPaczos · 2020-02-12T10:37:47Z

...igation-core/src/main/java/com/mapbox/navigation/core/telemetry/MapboxNavigationTelemetry.kt

+            .build()
+
+    // Call back that receives
+    private val routeProgressListener = object : RouteProgressObserver {


Still valid.

LukasPaczos · 2020-02-12T10:40:12Z

...igation-core/src/main/java/com/mapbox/navigation/core/telemetry/MapboxNavigationTelemetry.kt

+        /**
+         * Register a callback to receive location events. At most [MAX_LOCATION_VALUES] are stored
+         */
+        locationEngine.requestLocationUpdates(locationEngineRequest, locationCallback, null)


What was the legacy SDK's behavior? Were we collecting telemetry when navigation was not started? If we weren't we should listen from trip session updates instead.

LukasPaczos · 2020-02-12T10:42:00Z

settings.gradle

@@ -1,4 +1,4 @@
-include ':app',
+include ':app', ':libnavigation-telemetry',


There's no libnavigation-telemetry module.

Guardiola31337

@olegzil adding to the recent comments I left some other ones and questions.

Also, could you explain briefly how the approach we're taking is implemented? What's the overall APIs flow and architecture design? Could you provide examples / tests? I think that'd greatly help understanding the changes we're adding here and reviewing the PR will be easier.

libnavigation-core/src/main/java/com/mapbox/navigation/core/MapboxNavigation.kt

Guardiola31337 · 2020-02-18T21:17:02Z

libnavigation-core/src/main/java/com/mapbox/navigation/core/metrics/MapboxMetricsReporter.kt

@@ -1,12 +1,8 @@
-package com.mapbox.navigation.metrics
+package com.mapbox.navigation.core.metrics


Why did we move the code from libnavigation-metrics to libnavigation-core? Wouldn't adding libnavigation-metrics dependency in libnavigation-core work? If something needs to be changed, we should update in libnavigation-metrics, that's fine.

The telemetry package classes are marked internal. For core to use them, they have to be in the included

Guardiola31337 · 2020-02-18T21:25:19Z

libnavigation-core/src/main/java/com/mapbox/navigation/core/metrics/MapboxMetricsReporter.kt

 * Default implementation of [MetricsReporter] interface.
 */
-object MapboxMetricsReporter : MetricsReporter {
+class MapboxMetricsReporter(


This implementation directs the anonymized data to Mapbox metric pipelines and exposes an observer that allows for inspecting and collecting any events that are going to be pushed into the Mapbox pipeline. In other words, it's just a wrapper around MapboxTelemetry and handles MetricsObserver logic and lifetime. Does it need to be a class? Why not an object anymore?

👍 replaced

Guardiola31337 · 2020-02-18T21:55:11Z

...igation-core/src/main/java/com/mapbox/navigation/core/telemetry/MapboxNavigationTelemetry.kt

+        /**
+         * Register a callback to receive location events. At most [MAX_LOCATION_VALUES] are stored
+         */
+        locationEngine.requestLocationUpdates(locationEngineRequest, locationCallback, null)


No, we're not currently collecting navigation specific telemetry until a session has started. When it comes to location updates though, telemetry is collected by Telemetry core / Maps if the end user grants her consent.

Related to location updates, an interesting question was raised: for these before and after location buffers, do we want to keep track of raw location updates or the fix locations coming from NN? Currently, in the legacy we're collecting and sending the location updates generated by the LocationEngine but we probably want to gather the fixed ones. What do you think? cc @coxchapman

Guardiola31337 · 2020-02-18T22:02:57Z

libnavigation-core/src/main/java/com/mapbox/navigation/core/telemetry/NavigationNotification.kt

+ * Defines a contract in which a custom notification must adhere to when
+ * given to [com.mapbox.services.android.navigation.v5.navigation.MapboxNavigationOptions].
+ */
+interface NavigationNotification {


Why do we need this for Telemetry? 🤔

Guardiola31337 · 2020-02-18T22:08:01Z

...rc/main/java/com/mapbox/navigation/core/telemetry/telemetryevents/TelemetryDepartureEvent.kt

+// Defaulted values are optional
+
+@SuppressLint("ParcelCreator")
+class TelemetryDepartureEvent(


Same as above NavigationDepartEvent

NavigationEvent have been replaced by TelemetryEvent

Guardiola31337 · 2020-02-18T22:11:04Z

...-core/src/main/java/com/mapbox/navigation/core/telemetry/telemetryevents/TelemetryReroute.kt

+// Defaulted values are optional
+
+@SuppressLint("ParcelCreator")
+class TelemetryReroute(


Same as above NavigationRerouteEvent

NavigationEvent have been replaced by TelemetryEvent

Guardiola31337 · 2020-02-18T22:11:27Z

...ion-core/src/main/java/com/mapbox/navigation/core/telemetry/telemetryevents/TelemetryStep.kt

+
+// Defaulted values are optional
+
+data class TelemetryStep(


Same as above NavigationStepData

TelemetryStep data is required for some of the feedback types. NavigationStepData no longer exists.

NavigationEvent have been replaced by TelemetryEvent

Guardiola31337 · 2020-02-18T22:11:46Z

.../src/main/java/com/mapbox/navigation/core/telemetry/telemetryevents/TelemetryUserFeedback.kt

+
+// Defaulted values are optional
+@SuppressLint("ParcelCreator")
+class TelemetryUserFeedback(


Same as above NavigationFeedbackEvent

Outdated file. NavigationFeedbackEvent no longer exists. TelemetryFeedbackEvent is used instead.

All NavigationEvent have been replaced by TelemetryEvent

Guardiola31337 · 2020-02-18T22:14:30Z

libnavigation-util/src/main/java/com/mapbox/navigation/utils/audio/AudioTypeChain.kt

@@ -0,0 +1,16 @@
+package com.mapbox.navigation.core.telemetry.audio


Package directive doesn't match file location

Same thing applies to other classes 👇

This https://github.com/mapbox/mapbox-navigation-android/pull/2445/files#r380967783 still applies.

👍 removed

Guardiola31337 · 2020-02-18T23:41:33Z

Also there are some conflicts (e.g. this https://github.com/mapbox/mapbox-navigation-android/pull/2445/files#diff-e3c60303e8ee1a7d01f2af9369235fa9R25 already changed in master) so rebase from master when you have a chance, that'll also help making the PR easier to review 🚀

and user feedback events not being sent, if they where received in succession

…e_called_with_bearings tests not working becasue of because of MapboxNavigationTelemetry#unregisterListeners initializer = initializerDelegate 777ebfa

Guardiola31337 · 2020-03-11T03:30:28Z

Great work @olegzil 🚀

Going ahead and merging here. Will ticket out the tailwork.

cc @zugaldia

olegzil requested review from Guardiola31337, Lebedinsky, LukasPaczos, RingerJK and abhishek1508 January 31, 2020 21:55

Guardiola31337 reviewed Feb 3, 2020

View reviewed changes

RingerJK reviewed Feb 3, 2020

View reviewed changes

korshaknn reviewed Feb 3, 2020

View reviewed changes

...igation-core/src/main/java/com/mapbox/navigation/core/telemetry/MapboxNavigationTelemetry.kt Outdated Show resolved Hide resolved

korshaknn reviewed Feb 3, 2020

View reviewed changes

...igation-core/src/main/java/com/mapbox/navigation/core/telemetry/MapboxNavigationTelemetry.kt Outdated Show resolved Hide resolved

LukasPaczos reviewed Feb 3, 2020

View reviewed changes

korshaknn approved these changes Feb 3, 2020

View reviewed changes

...igation-core/src/main/java/com/mapbox/navigation/core/telemetry/MapboxNavigationTelemetry.kt Outdated Show resolved Hide resolved

korshaknn self-requested a review February 3, 2020 19:37

korshaknn reviewed Feb 4, 2020

View reviewed changes

...igation-core/src/main/java/com/mapbox/navigation/core/telemetry/MapboxNavigationTelemetry.kt Outdated Show resolved Hide resolved

Guardiola31337 reviewed Feb 4, 2020

View reviewed changes

olegzil force-pushed the oz-telemetry-implementation branch from 46d8acf to f55d5b7 Compare February 4, 2020 21:37

olegzil marked this pull request as ready for review February 5, 2020 17:26

olegzil force-pushed the oz-telemetry-implementation branch from f55d5b7 to ac7f487 Compare February 5, 2020 19:21

RingerJK reviewed Feb 6, 2020

View reviewed changes

libnavigation-core/src/test/java/com/mapbox/navigation/core/ContextCreationHelpers.kt Outdated Show resolved Hide resolved

RingerJK reviewed Feb 6, 2020

View reviewed changes

libnavigation-core/src/test/java/com/mapbox/navigation/core/MapboxNavigationTest.kt Outdated Show resolved Hide resolved

RingerJK reviewed Feb 6, 2020

View reviewed changes

libnavigation-core/src/test/java/com/mapbox/navigation/core/MapboxNavigationTest.kt Outdated Show resolved Hide resolved

RingerJK reviewed Feb 6, 2020

View reviewed changes

...gation-core/src/main/java/com/mapbox/navigation/core/telemetry/TelemetryEventUserFeedback.kt Outdated Show resolved Hide resolved

olegzil force-pushed the oz-telemetry-implementation branch from 2c918b3 to dbea0ed Compare February 6, 2020 21:04

RingerJK approved these changes Feb 7, 2020

View reviewed changes

olegzil force-pushed the oz-telemetry-implementation branch from 5597613 to 2908021 Compare February 12, 2020 01:30

LukasPaczos reviewed Feb 12, 2020

View reviewed changes

LukasPaczos mentioned this pull request Feb 13, 2020

Add logic to determine faster route #2462

Merged

10 tasks

Guardiola31337 reviewed Feb 18, 2020

View reviewed changes

olegzil and others added 18 commits March 10, 2020 19:17

updated comments

0c73fdf

fix formatting

c1d2dd1

function rename

b7ab4b7

added missing data to the offroute event

4048e19

Fixes for multiple off-road

1375726

and user feedback events not being sent, if they where received in succession

metadata fixes

a1b9d85

fix formatting and naming

c57d93f

fix mapbox navigation accounts sku token logic

32c9651

fix for pre/post location buffers

dd0bf81

re-fix reroute count

f9d98fc

added code to send all events in response to onDestroy()

a10b6e9

added code to distinguish between arrival and a cancled session

9d6f49b

added code to record metadata values that start with 'origina'

6f47507

added correct original distance calculation

94f5f51

fix formatting

20d0984

added callback to get initial location

d1c8def

added code to wait for a first location event using CompletableDeffered.

fc3737c

fix formatting and remove temporarily faster route events logic

9584a6a

Guardiola31337 force-pushed the oz-telemetry-implementation branch from 16ca439 to 9584a6a Compare March 10, 2020 23:17

olegzil and others added 6 commits March 10, 2020 17:17

request ID and final event posting fixes

09aef08

request ID and final event posting fixes

97135d7

adding call to clear all vars

087ebc4

unregister fix

777ebfa

final fix for session start

41a2b69

ignore mapbox navigation reRoute_called_with_null_bearings and reRout…

965f9c1

…e_called_with_bearings tests not working becasue of because of MapboxNavigationTelemetry#unregisterListeners initializer = initializerDelegate 777ebfa

Guardiola31337 force-pushed the oz-telemetry-implementation branch from 83fa365 to 965f9c1 Compare March 11, 2020 03:18

Guardiola31337 merged commit 9dbc0da into master Mar 11, 2020

Guardiola31337 mentioned this pull request Mar 19, 2020

Fix events generation and ingestion #2616

Merged

10 tasks

Guardiola31337 mentioned this pull request Jun 12, 2020

Telemetry tailwork #3164

Closed

5 tasks

		@@ -1,4 +1,4 @@
		include ':app',
		include ':app', ':libnavigation-telemetry',

		@@ -1,12 +1,8 @@
		package com.mapbox.navigation.metrics
		package com.mapbox.navigation.core.metrics

		@@ -0,0 +1,16 @@
		package com.mapbox.navigation.core.telemetry.audio

initial commit of the new telemetry handler #2445

initial commit of the new telemetry handler #2445

Uh oh!

Conversation

olegzil commented Jan 31, 2020

Description

Goal

Implementation

Screenshots or Gifs

Testing

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

olegzil Feb 3, 2020 • 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!

Uh oh!

Uh oh!

Uh oh!

RingerJK left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LukasPaczos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

olegzil commented Feb 3, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

olegzil Feb 3, 2020 •

edited

Loading

olegzil Feb 24, 2020 •

edited

Loading