MapboxNavigationTelemetry refactoring #3540

korshaknn · 2020-09-04T15:57:09Z

Description

Let's simplify code inside TelemetryLocationAndProgressDispatcher and MapboxNavigationTelemetry

How it works:
MapboxNavigationTelemetry is used to post NavigationEvent.

NavigationAppUserTurnstileEvent is sent when MapboxNavigationTelemetry is initialized
NavigationDepartEvent is sent when session state changes to ACTIVE_GUIDANCE
NavigationRerouteEvent is sent after reroute
NavigationArriveEvent is sent after arrival
NavigationCancelEvent is sent when session state changes to FREE_DRIVE or IDLE, or when parent job is canceled.
NavigationFeedbackEvent might sent at any time between depart and arrive/cancel.

To populate event with data, TelemetryLocationAndProgressDispatcher is used. It provides all needed data such as original/new route, routeProgress, location, etc.

All events are sent immediately, but NavigationRerouteEvent and NavigationFeedbackEvent are pending. It means that we need a list of locations before and after each event.
TelemetryLocationAndProgressDispatcher caches the most recent locations. Max size of the cache is 20.
When any of pending events is fired, we get the cache and copy it to the event as preEventLocations and start to collect postEventLocations. When it reaches the max size (20 items) we send pending event.
In perfect situation pending event will have 20 locations before event and 20 locations after.
If we arrive to the destination or somehow cancel the session (go to FREE_DRIVE or cancel the parent job), we don't wait for all 20 postEventLocations, we post it immediately.

The base flow

init -> send NavigationAppUserTurnstileEvent
active guidance -> send NavigationDepartEvent
user pressed feedback -> NavigationFeedbackEvent is not sent, collect locations, when ready -> send
user pressed feedback -> NavigationFeedbackEvent is not sent, collect locations, when ready -> send
user pressed feedback -> NavigationRerouteEvent is not sent, collect locations, when ready -> send
arrive -> send all pending event if they have not been sent yet and also send NavigationArriveEvent

If we set a new route externally, we send all pending events, send NavigationCancelEvent and NavigationDepartEvent

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
We might need to update / push api/current.txt files after running $> make core-update-api (Core) / $> make ui-update-api (UI) if there are changes / errors we're 🆗 with (e.g. AddedMethod changes are marked as errors but don't break SemVer) 🚀 If there are SemVer breaking changes add the SEMVER label. See Metalava docs

Guardiola31337

Amazing work here and nice test coverage @korshaknn 💪

Some comments / questions to discuss before merging here.

NavigationFeedbackEvent might sent at any time between depart and arrive/cancel.

We should allow sending feedback events after arrival.

If we arrive to the destination or somehow cancel the session (go to FREE_DRIVE or cancel the parent job), we don't wait for all 20 postEventLocations, we post it immediately.

It's also happening with preEventLocations, right?

Also wondering if we should wait until the session is canceled or the buffers are full. If not, we have a high chance to lose locations. What do you think?

2020-10-19 16:59:27.754 10667-10667/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: collect post event locations for user feedback
2020-10-19 16:59:28.093 10667-10667/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: collect post event locations for user feedback
2020-10-19 16:59:28.409 10667-10667/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: collect post event locations for user feedback
2020-10-19 16:59:28.728 10667-10667/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 16:59:29.722 10667-10667/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 16:59:30.757 10667-10667/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 16:59:31.739 10667-10667/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 16:59:32.740 10667-10667/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 16:59:33.785 10667-10667/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 16:59:34.727 10667-10667/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 16:59:35.737 10667-10667/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 16:59:36.736 10667-10667/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 16:59:37.815 10667-10667/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 16:59:38.086 10667-10667/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: Navigation state is FREE_DRIVE
2020-10-19 16:59:38.086 10667-10667/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: onRoutesChanged received. Route list size = 0
2020-10-19 16:59:38.108 10667-10667/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: sessionStop
2020-10-19 16:59:38.108 10667-10667/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: handleSessionCanceled
2020-10-19 16:59:38.108 10667-10667/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: flushing eventsLocationsBuffer. Pending events = 3
2020-10-19 16:59:38.116 10667-10667/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: class com.mapbox.navigation.core.telemetry.events.NavigationFeedbackEvent event sent
2020-10-19 16:59:38.134 10667-10667/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: class com.mapbox.navigation.core.telemetry.events.NavigationFeedbackEvent event sent
2020-10-19 16:59:38.142 10667-10667/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: class com.mapbox.navigation.core.telemetry.events.NavigationCancelEvent event sent

Tried to send 3 feedback events after arrival and only 2 were flushed. Note

2020-10-19 16:59:38.108 10667-10667/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: flushing eventsLocationsBuffer. Pending events = 3

Same issue different numbers 👀

2020-10-19 17:05:32.351 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: collect post event locations for user feedback
2020-10-19 17:05:32.667 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: collect post event locations for user feedback
2020-10-19 17:05:33.671 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 17:05:34.680 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 17:05:35.693 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 17:05:36.680 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 17:05:37.662 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 17:05:38.700 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 17:05:39.723 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 17:05:40.684 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 17:05:41.680 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 17:05:42.708 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 17:05:43.664 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 17:05:44.700 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 17:05:45.685 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 17:05:45.888 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: collect post event locations for user feedback
2020-10-19 17:05:46.155 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: collect post event locations for user feedback
2020-10-19 17:05:46.670 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 17:05:47.686 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 17:05:48.671 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 17:05:49.665 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 17:05:50.697 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: route progress state = ROUTE_COMPLETE
2020-10-19 17:05:51.671 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: Navigation state is IDLE
2020-10-19 17:05:51.671 22859-22859/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: MapboxNavigation onDestroy
2020-10-19 17:05:51.809 22859-22909/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: master job canceled
2020-10-19 17:05:51.809 22859-22909/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: sessionStop
2020-10-19 17:05:51.810 22859-22909/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: handleSessionCanceled
2020-10-19 17:05:51.810 22859-22909/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: flushing eventsLocationsBuffer. Pending events = 4
2020-10-19 17:05:51.821 22859-22909/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: class com.mapbox.navigation.core.telemetry.events.NavigationFeedbackEvent event sent
2020-10-19 17:05:51.844 22859-22909/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: class com.mapbox.navigation.core.telemetry.events.NavigationFeedbackEvent event sent
2020-10-19 17:05:51.850 22859-22909/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: class com.mapbox.navigation.core.telemetry.events.NavigationCancelEvent event sent

This was closing the activity (pressing back button) previous one was clicking on CLEAR ROUTES. Sent 4 feedback events 👀

2020-10-19 17:05:51.810 22859-22909/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: flushing eventsLocationsBuffer. Pending events = 4
2020-10-19 17:05:51.821 22859-22909/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: class com.mapbox.navigation.core.telemetry.events.NavigationFeedbackEvent event sent
2020-10-19 17:05:51.844 22859-22909/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: class com.mapbox.navigation.core.telemetry.events.NavigationFeedbackEvent event sent
2020-10-19 17:05:51.850 22859-22909/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: class com.mapbox.navigation.core.telemetry.events.NavigationCancelEvent event sent

It says pending events 4 and only 2 were sent 🤔

Can we add a test for that?

Currently (AFAIK), in case we have a route with multiple way points we don't send a DEPART and CANCEL event at each pair of waypoints.

This is not supported at the moment. We should figure how to solve as we're not calling setRoute 🤔

Is #2748 going to be addressed as part of this PR? It'd be great to have a test for that.

Guardiola31337 · 2020-10-19T21:28:44Z

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

+                // TODO:OZ voiceIndex is not available in SDK 1.0 and was not set in the legacy telemetry        navigationEvent.voiceIndex
+                // TODO:OZ bannerIndex is not available in SDK 1.0 and was not set in the legacy telemetry        navigationEvent.bannerIndex


Are these TODOs fixed?

I don't know what to do with it.
There are no voiceIndex or bannerIndex properties in the model classes.
Looks like can be removed.

mapbox-navigation-android/libnavigation-core/src/main/java/com/mapbox/navigation/core/telemetry/events/NavigationEvent.kt

Lines 66 to 67 in a3dfcaa

var voiceIndex: Int = 0

var bannerIndex: Int = 0

Although it's true that in the legacy we were not setting them either

mapbox-navigation-android/libandroid-navigation/src/main/java/com/mapbox/services/android/navigation/v5/navigation/metrics/NavigationEvent.java

Lines 369 to 383 in e232efd

int getVoiceIndex() {

return voiceIndex;

}

void setVoiceIndex(int voiceIndex) {

this.voiceIndex = voiceIndex;

}

int getBannerIndex() {

return bannerIndex;

}

void setBannerIndex(int bannerIndex) {

this.bannerIndex = bannerIndex;

}

(these are optional). Maybe it's a good opportunity to start sending them. What do you think?

yep, NavigationEvent has such fields, but what data should be used to init them?
something from RouteProgress?

Guardiola31337 · 2020-10-19T21:34:29Z

...main/java/com/mapbox/navigation/core/telemetry/TelemetryLocationAndProgressDispatcherImpl.kt

+    override var firstLocation: Location? = null
+    override var routeProgress: RouteProgress? = null
+    override var originalRoute = CompletableDeferred<DirectionsRoute>()
+    private val mutex = Mutex()


Do we really need this Mutex?

yes. we remove items with iterator and it should be synchronized. fun is suspend, so Mutex is used for synchronization.

private suspend fun accumulatePostEventLocation(location: Location) { mutex.withLock { val iterator = eventsLocationsBuffer.iterator() while (iterator.hasNext()) { iterator.next().let { it.addPostEventLocation(location) if (it.postEventLocationsSize() >= LOCATION_BUFFER_MAX_SIZE) { it.onBufferFull() iterator.remove() } } } } }

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

...test/java/com/mapbox/navigation/core/telemetry/TelemetryLocationAndProgressDispatcherTest.kt

korshaknn · 2020-10-20T09:34:10Z

@Guardiola31337

We should allow sending feedback events after arrival.

we can do it, but it will break the order. Right now all feedback events will be between depart-arrive, so we have chronological order. If we allow to send it after arrival order will be broken.

It's also happening with preEventLocations, right?

no, we always take preEventLocations as is. If there are 20 - take 20. If there are 1 - take 1.

Also wondering if we should wait until the session is canceled or the buffers are full. If not, we have a high chance to lose locations. What do you think?

It depends on:

do we need a chronological order of all events?
can we make all events pending (all of them will wait for 20 post locations or session cancel) or we need to have depart and arrive as immediate events? Right now they are immediate. But if we make them pending, order will be saved.

It says pending events 4 and only 2 were sent 🤔
Can we add a test for that?

I've retested it many times in different scenarios. Everything works fine. Are you using the latest commit?
btw, there are tests to check that case:
feedback_and_reroute_events_sent_on_arrival, feedback_and_reroute_events_sent_on_free_drive, feedback_and_reroute_events_sent_on_master_job_canceled

let's discuss all multiwayPoint logic when the PR will be merged, to many details right now )

Guardiola31337 · 2020-10-20T15:58:10Z

Discussed internally.

We should allow sending feedback events after arrival.

we can do it, but it will break the order. Right now all feedback events will be between depart-arrive, so we have chronological order. If we allow to send it after arrival order will be broken.

That's currently an option. In any case, we're going to keep track of the created timestamp (set it when it's created) and try to collect as much as preEventLocations and postEventLocations as possible (if the queues are full, feedback / reroute events will be sent immediately) and only flush the pending events (as is i.e. preEventLocations and postEventLocations collected until that moment) if:

the session is canceled
the parent job is canceled
a new route is set

so the sending order won't be a problem.

It's also happening with preEventLocations, right?

no, we always take preEventLocations as is. If there are 20 - take 20. If there are 1 - take 1.

See comment above:

try to collect as much as preEventLocations and postEventLocations as possible (if the queues are full, feedback / reroute events will be sent immediately) and only flush the pending events (as is i.e. preEventLocations and postEventLocations collected until that moment)

Also wondering if we should wait until the session is canceled or the buffers are full. If not, we have a high chance to lose locations. What do you think?

It depends on:

do we need a chronological order of all events?
can we make all events pending (all of them will wait for 20 post locations or session cancel) or we need to have depart and arrive as immediate events? Right now they are immediate. But if we make them pending, order will be saved.

See comment above. If we keep track of the creation time, order in which events are pushed shouldn't be an issue. We can keep pushing depart, arrive and cancel events immediately and push feedback and reroute ones afterwards (with the logic above-mentioned).

It says pending events 4 and only 2 were sent 🤔

I've retested it many times in different scenarios. Everything works fine. Are you using the latest commit?

This is still reproducible. We should investigate further and fix.

let's discuss all multiwayPoint logic when the PR will be merged, to many details right now )

Sounds good 👍

Guardiola31337

Is it safe to remove internal TelemetryEvent and SessionState? It seems so.

korshaknn · 2020-10-21T11:28:42Z

Is it safe to remove internal TelemetryEvent and SessionState? It seems so.

it was unused

Guardiola31337 · 2020-10-21T13:43:14Z

it was unused

We should remove them then 😅

LukasPaczos

Fantastic work @korshaknn!

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

LukasPaczos · 2020-10-21T16:48:55Z

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

+    }
+
+    @Test
+    fun feedback_and_reroute_events_not_sent_on_arrival() = runBlocking {


Why should we not send those events on arrival?

#3540 (review)

Also wondering if we should wait until the session is canceled or the buffers are full. If not, we have a high chance to lose locations.

#3540 (comment)

try to collect as much as preEventLocations and postEventLocations as possible (if the queues are full, feedback / reroute events will be sent immediately) and only flush the pending events (as is i.e. preEventLocations and postEventLocations collected until that moment) if:

the session is canceled

the parent job is canceled

a new route is set

What happens if the user arrives and force-closes the app without going to free-drive? Are we going to lose the events?

I think that would be too big of a risk to rely on developers to go to free-drive after arriving or to wait for the buffer to fill up in order to receive the events. I'd personally rather lose some post-event location samples than the whole event.

no no, everything will be ok. it will be handled by parentJob listener.

What happens if the user arrives and force-closes the app without going to free-drive? Are we going to lose the events?

Nah, if that happens, all pending events are sent immediately.

only flush the pending events (as is i.e. preEventLocations and postEventLocations collected until that moment) if:

the session is canceled

the parent job is canceled

a new route is set

Does that make sense? If the implementation is not behaving like that, then definitely is a bug that we need to fix.

cc @korshaknn

LukasPaczos · 2020-10-21T16:51:43Z

...test/java/com/mapbox/navigation/core/telemetry/TelemetryLocationAndProgressDispatcherTest.kt

+        callbackDispatcher.onOffRouteStateChanged(true)
+        callbackDispatcher.onOffRouteStateChanged(false)
+        callbackDispatcher.onRoutesChanged(routes)


Should this race be resolved in the core module?

what do you mean?
we use it to simulate behavior and test some flow

LukasPaczos · 2020-10-21T16:53:47Z

...test/java/com/mapbox/navigation/core/telemetry/TelemetryLocationAndProgressDispatcherTest.kt

+        callbackDispatcher.onRoutesChanged(routes)
+        assertEquals(route, (callbackDispatcher.newRouteChannel.receive() as ExternalRoute).route)


Is this correct? Since we are still not on the route (at least Nav Native doesn't think we are), shouldn't this be considered a reroute-route?

it depends on onOffRouteStateChanged.
if it was called with offRoute==true, we will use the next new route as RerouteRoute, or ExternalRoute otherwise

...main/java/com/mapbox/navigation/core/telemetry/TelemetryLocationAndProgressDispatcherImpl.kt

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

Guardiola31337 · 2020-10-21T17:09:18Z

It seems feedback_and_reroute_events_sent_on_master_job_canceled - com.mapbox.navigation.core.telemetry.MapboxNavigationTelemetryTest is flaky https://app.circleci.com/pipelines/github/mapbox/mapbox-navigation-android/9122/workflows/67cc4d7d-cd4e-4106-bf26-7cc645e41b72/jobs/33995 😥

Guardiola31337 · 2020-10-21T18:03:19Z

Noticed that we're trying to send cancel and then send an incorrect depart when selecting an alternative route but the session hasn't started 👀

2020-10-21 13:59:00.254 10332-10332/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: MapboxMetricsReporter.init from MapboxNavigation main
2020-10-21 13:59:00.261 10332-10332/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: onOffRouteStateChanged false
2020-10-21 13:59:00.261 10332-10332/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: Navigation state is IDLE
2020-10-21 13:59:00.263 10332-10332/com.mapbox.navigation.examples I/MAPBOX_TELEMETRY: Valid initialization
2020-10-21 13:59:04.711 10332-10332/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: onRoutesChanged received. Route list size = 2
2020-10-21 13:59:13.142 10332-10332/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: onRoutesChanged received. Route list size = 2
2020-10-21 13:59:13.153 10332-10332/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: handleExternalRoute
2020-10-21 13:59:13.153 10332-10332/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: sessionStop
2020-10-21 13:59:13.153 10332-10332/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: handleSessionCanceled
2020-10-21 13:59:13.154 10332-10332/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: flushing eventsLocationsBuffer. Pending events = 0
2020-10-21 13:59:13.161 10332-10332/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: populateNavigationEvent
2020-10-21 13:59:13.162 10332-10332/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: class com.mapbox.navigation.core.telemetry.events.NavigationCancelEvent not sent. Caused by: Navigation Session started: false. Route exists: true
2020-10-21 13:59:13.163 10332-10332/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: sessionStart
2020-10-21 13:59:13.164 10332-10332/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: flushing eventsLocationsBuffer. Pending events = 0
2020-10-21 13:59:13.171 10332-10332/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: populateNavigationEvent
2020-10-21 13:59:13.172 10332-10332/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: class com.mapbox.navigation.core.telemetry.events.NavigationDepartEvent event sent
2020-10-21 14:00:08.612 10332-10332/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: MapboxNavigation onDestroy
2020-10-21 14:00:08.686 10332-10513/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: master job canceled
2020-10-21 14:00:08.686 10332-10513/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: sessionStop
2020-10-21 14:00:08.686 10332-10513/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: handleSessionCanceled
2020-10-21 14:00:08.686 10332-10513/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: flushing eventsLocationsBuffer. Pending events = 0
2020-10-21 14:00:08.696 10332-10513/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: populateNavigationEvent
2020-10-21 14:00:08.697 10332-10513/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: class com.mapbox.navigation.core.telemetry.events.NavigationCancelEvent event sent

We should make sure that events are only pushed when in Active Guidance.

korshaknn · 2020-10-21T18:23:38Z

we can get a new route after:

original is selected
alternative selected
external is set
offroute
don't have enough info to split external from alternative

if we use
if (sessionStarted) {}
we loose external routes that are set when session is NOT active
BUT if we don't use that condition ^^ every alternative route (or just a new route after long-click) will fire session start and depart event

Guardiola31337 · 2020-10-21T19:09:51Z

Couldn't we have that logic irrespective of the state but only send the events if the session has started? We had telemetryEventGate or something like that which was checking if the session was started to send the events to to the backend. Would something similar work?

korshaknn · 2020-10-23T10:45:12Z

@LukasPaczos @Guardiola31337 ready for the next round

Guardiola31337 · 2020-10-23T16:16:24Z

Noticed that we're trying to send a NavigationCancelEvent and we're sending an incorrect (extra) NavigationDepartEvent when following the next steps in SimpleMapboxNavigationKt

Select a route (long press)
Click on CLEAR ROUTES button
Click on START NAVIGATION button (Free Drive)
Click on ADD ORIGINAL ROUTE button (Active Guidance)

2020-10-23 12:12:53.738 27465-27465/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: onRoutesChanged received. Route list size = 0
2020-10-23 12:12:55.023 27465-27465/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: Navigation state is FREE_DRIVE
2020-10-23 12:12:58.490 27465-27465/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: Navigation state is ACTIVE_GUIDANCE
2020-10-23 12:12:58.490 27465-27465/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: onRoutesChanged received. Route list size = 1
2020-10-23 12:12:58.508 27465-27465/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: sessionStart
2020-10-23 12:12:58.509 27465-27465/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: handleExternalRoute
2020-10-23 12:12:58.509 27465-27465/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: sessionStop
2020-10-23 12:12:58.509 27465-27465/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: handleSessionCanceled
2020-10-23 12:12:58.509 27465-27465/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: flushing eventsLocationsBuffer. Pending events = 0
2020-10-23 12:12:58.516 27465-27465/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: populateNavigationEvent
2020-10-23 12:12:58.517 27465-27465/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: class com.mapbox.navigation.core.telemetry.events.NavigationCancelEvent not sent. Caused by: Navigation Session started: false. Route exists: true
2020-10-23 12:12:58.518 27465-27465/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: sessionStart
2020-10-23 12:12:58.536 27465-27465/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: flushing eventsLocationsBuffer. Pending events = 0
2020-10-23 12:12:58.541 27465-27465/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: populateNavigationEvent
2020-10-23 12:12:58.542 27465-27465/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: class com.mapbox.navigation.core.telemetry.events.NavigationDepartEvent event sent
2020-10-23 12:12:58.543 27465-27465/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: flushing eventsLocationsBuffer. Pending events = 0
2020-10-23 12:12:58.548 27465-27465/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: populateNavigationEvent
2020-10-23 12:12:58.548 27465-27465/com.mapbox.navigation.examples D/MAPBOX_TELEMETRY: class com.mapbox.navigation.core.telemetry.events.NavigationDepartEvent event sent

Guardiola31337 · 2020-10-26T17:52:15Z

Took latest changes for a spin and all the tests cases / scenarios run were successful 🎉

Great work removing Coroutines / suspend and simplifying the overall logic - it seems they were adding unnecessary complexity 💪

Let's work on tests and the tailwork 🚀

Guardiola31337

Run some tests and so far it's working as expected 🚀

Left some minor comments / questions.

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

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

Guardiola31337 · 2020-10-27T21:39:12Z

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

+        return this.apply {
+            sdkIdentifier = this@MapboxNavigationTelemetry.sdkIdentifier
+
+            routeProgress!!.let { routeProgress ->


Should we check for routeProgress and originalRoute first to populate all the data? Wondering if we could run into a situation in which routeProgress and originalRoute are null but the event is sent with missing / partial data 🤔

no need to check it.
NavigationEvent.populate() can be called only if the session is started.
And session can be started when routeProgress and originalRoute are not null.

Wondering if we could run into a situation in which routeProgress and originalRoute are null but the event is sent with missing / partial data 🤔

if any of them is null we will have a crash, no event will be sent.

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

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

Guardiola31337

Thanks for addressing the feedback @korshaknn

Great great work 💪

Let's merge here and follow up with any tailwork in separate future PRs 🚀

korshaknn requested a review from Guardiola31337 September 4, 2020 15:57

korshaknn self-assigned this Sep 4, 2020

korshaknn force-pushed the knn-telemetry-fixes branch 3 times, most recently from 454fd80 to 6dd137a Compare September 8, 2020 14:05

korshaknn force-pushed the knn-telemetry-fixes branch 2 times, most recently from 36bc018 to f299e88 Compare September 18, 2020 14:17

korshaknn force-pushed the knn-telemetry-fixes branch 2 times, most recently from 9a5c314 to 4c4f3ee Compare October 1, 2020 14:05

korshaknn force-pushed the knn-telemetry-fixes branch from e7ca96c to 135b092 Compare October 8, 2020 11:14

korshaknn marked this pull request as ready for review October 8, 2020 11:50

korshaknn requested a review from LukasPaczos October 8, 2020 11:50

korshaknn added navigation-core labels Oct 8, 2020

korshaknn added this to the v1.2.0 milestone Oct 8, 2020

korshaknn requested review from Lebedinsky and RingerJK October 9, 2020 10:13

Guardiola31337 reviewed Oct 19, 2020

View reviewed changes

Guardiola31337 reviewed Oct 20, 2020

View reviewed changes

korshaknn force-pushed the knn-telemetry-fixes branch 2 times, most recently from 1e9cc0d to 7bb35c1 Compare October 21, 2020 16:32

LukasPaczos reviewed Oct 21, 2020

View reviewed changes

korshaknn force-pushed the knn-telemetry-fixes branch 4 times, most recently from e349c71 to 8e3c78f Compare October 22, 2020 18:21

Guardiola31337 mentioned this pull request Oct 23, 2020

ReRouteActivity LocationComponent has to be activated Robo test crash #3404

Closed

kmadsen mentioned this pull request Oct 23, 2020

Telemetry distancecompleted can be impossible large #3682

Closed

Guardiola31337 mentioned this pull request Oct 27, 2020

Firebase robo crash NoSuchFieldException: HTTP_SERVER #3645

Closed

Guardiola31337 reviewed Oct 27, 2020

View reviewed changes

LukasPaczos reviewed Oct 28, 2020

View reviewed changes

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

Guardiola31337 reviewed Oct 28, 2020

View reviewed changes

...ion-core/src/test/java/com/mapbox/navigation/core/telemetry/MapboxNavigationTelemetryTest.kt Outdated Show resolved Hide resolved

Telemetry refactoring. removed coroutines, simplified code

4569b57

korshaknn force-pushed the knn-telemetry-fixes branch from 07473c5 to 4569b57 Compare October 28, 2020 14:52

Guardiola31337 approved these changes Oct 28, 2020

View reviewed changes

korshaknn merged commit ded3fd2 into master Oct 28, 2020

This was referenced Oct 28, 2020

Telemetry tailwork #3164

Closed

Fix flaky telemetry tests #3707

Merged

korshaknn deleted the knn-telemetry-fixes branch November 12, 2020 16:09

Guardiola31337 mentioned this pull request Dec 1, 2021

Fix Events distanceTraveled not being accumulated #5191

Merged

		// TODO:OZ voiceIndex is not available in SDK 1.0 and was not set in the legacy telemetry navigationEvent.voiceIndex
		// TODO:OZ bannerIndex is not available in SDK 1.0 and was not set in the legacy telemetry navigationEvent.bannerIndex

	int getVoiceIndex() {
	return voiceIndex;
	}

	void setVoiceIndex(int voiceIndex) {
	this.voiceIndex = voiceIndex;
	}

	int getBannerIndex() {
	return bannerIndex;
	}

	void setBannerIndex(int bannerIndex) {
	this.bannerIndex = bannerIndex;
	}

		callbackDispatcher.onRoutesChanged(routes)
		assertEquals(route, (callbackDispatcher.newRouteChannel.receive() as ExternalRoute).route)

MapboxNavigationTelemetry refactoring #3540

MapboxNavigationTelemetry refactoring #3540

Uh oh!

Conversation

korshaknn commented Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Checklist

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!

korshaknn Oct 20, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

korshaknn commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Guardiola31337 commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Guardiola31337 left a comment

Choose a reason for hiding this comment

Uh oh!

korshaknn commented Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Guardiola31337 commented Oct 21, 2020

Uh oh!

LukasPaczos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Guardiola31337 Oct 21, 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!

Choose a reason for hiding this comment

Uh oh!

Guardiola31337 Oct 21, 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!

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!

Guardiola31337 commented Oct 21, 2020

Uh oh!

Guardiola31337 commented Oct 21, 2020

Uh oh!

korshaknn commented Oct 21, 2020

Uh oh!

Guardiola31337 commented Oct 21, 2020

Uh oh!

korshaknn commented Oct 23, 2020

korshaknn commented Sep 4, 2020 •

edited

Loading

korshaknn Oct 20, 2020 •

edited

Loading

korshaknn commented Oct 20, 2020 •

edited

Loading

Guardiola31337 commented Oct 20, 2020 •

edited

Loading

korshaknn commented Oct 21, 2020 •

edited

Loading

Guardiola31337 Oct 21, 2020 •

edited

Loading

Guardiola31337 Oct 21, 2020 •

edited

Loading

Guardiola31337 commented Oct 23, 2020 •

edited

Loading