Core telemetry migration #6423

RingerJK · 2022-09-30T17:32:46Z

Description

migrating to core telemetry

~~blocked by #6422~~

...src/main/java/com/mapbox/navigation/core/telemetry/events/NavigationAppUserTurnstileEvent.kt

...tion-core/src/main/java/com/mapbox/navigation/core/telemetry/events/NavigationCancelEvent.kt

...avigation-core/src/test/java/com/mapbox/navigation/core/telemetry/events/EventsTestHelper.kt

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

libnavigation-metrics/src/main/java/com/mapbox/navigation/metrics/extensions/MetricEventEx.kt

publish_android_snapshot

codecov · 2022-10-03T22:43:49Z

Codecov Report

Merging #6423 (2c9df75) into main (0e32735) will increase coverage by 0.06%.
The diff coverage is 69.33%.

@@             Coverage Diff              @@
##               main    #6423      +/-   ##
============================================
+ Coverage     69.42%   69.49%   +0.06%     
- Complexity     4653     4707      +54     
============================================
  Files           697      700       +3     
  Lines         27520    27703     +183     
  Branches       3205     3255      +50     
============================================
+ Hits          19107    19251     +144     
+ Misses         7169     7166       -3     
- Partials       1244     1286      +42

Impacted Files	Coverage Δ
...ava/com/mapbox/navigation/core/MapboxNavigation.kt	`67.57% <0.00%> (ø)`
...navigation/core/telemetry/NavEventsPopulateUtil.kt	`94.59% <ø> (ø)`
...box/navigation/core/telemetry/events/PhoneState.kt	`100.00% <ø> (ø)`
...vigation/metrics/internal/EventsServiceProvider.kt	`0.00% <0.00%> (ø)`
...ation/metrics/internal/TelemetryServiceProvider.kt	`0.00% <0.00%> (ø)`
...igation/metrics/internal/TelemetryUtilsDelegate.kt	`0.00% <0.00%> (ø)`
...n/core/telemetry/events/NavigationFeedbackEvent.kt	`72.00% <41.66%> (-28.00%)`	⬇️
...mapbox/navigation/metrics/MapboxMetricsReporter.kt	`54.00% <47.22%> (-10.00%)`	⬇️
...on/core/telemetry/events/NavigationRerouteEvent.kt	`82.60% <66.66%> (-9.06%)`	⬇️
.../core/telemetry/events/NavigationFreeDriveEvent.kt	`82.35% <70.83%> (-4.32%)`	⬇️
... and 16 more

libnavigation-metrics/src/test/java/com/mapbox/navigation/metrics/MapboxMetricsReporterTest.kt

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

...on-core/src/main/java/com/mapbox/navigation/core/telemetry/events/CoreTelemetryEventUtils.kt

...rics/src/main/java/com/mapbox/navigation/metrics/internal/event/AppUserTurnstileInterface.kt

libnavigation-base/api/current.txt

build.gradle

LukasPaczos · 2022-10-05T15:48:35Z

gradle/dependencies.gradle

      mapboxSdkDirectionsModels : "com.mapbox.mapboxsdk:mapbox-sdk-directions-models:${version.mapboxSdkServices}",
      mapboxSdkRefreshModels    : "com.mapbox.mapboxsdk:mapbox-sdk-directions-refresh-models:${version.mapboxSdkServices}",
-      mapboxEvents              : "com.mapbox.mapboxsdk:mapbox-android-telemetry:${version.mapboxEvents}",
      mapboxCore                : "com.mapbox.mapboxsdk:mapbox-android-core:${version.mapboxCore}",


This can also be removed, doesn't it?

looks like we can, let me check

we cannot, following classes are part of mapbox-android-core:

com.mapbox.android.core.location.LocationEngine;

com.mapbox.android.core.location.LocationEngineRequest

Weren't we planning to reintroduce this same classes under the same packages but directly from the Common SDK @tarigo @tatiana-yan?

In the current situation, we'd still need to maintain https://github.com/mapbox/mapbox-events-android/.

we have com.mapbox.common.location.compat.LocationEngine, com.mapbox.common.location.compat.LocationEngineRequest

I see https://github.com/mapbox/mapbox-sdk-common/issues/2807 is still open (I can't find corresponding JIRA ticket).

This means that Nav SDK cannot drop the dependency on MME (at least the location part) because LocationEngine and LocationEngineRequest are part of our public API.

Let's discuss this separately, it doesn't block the PR.

For the external customers there is a plan to replace it by renaming Common's LocationEngine compat, you can keep core dependency because it's a public API but migrate to compat - it would be great to test it, but from the next release - there are some issues to address.
Maps SDK has already adopted it.

ref CORESDK-465

libnavigation-base/src/main/java/com/mapbox/navigation/base/metrics/MetricEvent.kt

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

...tion-core/src/main/java/com/mapbox/navigation/core/telemetry/events/NavigationCancelEvent.kt

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

tatiana-yan · 2022-10-06T07:26:55Z

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


    /**
-     * Disables metrics reporting and ends [mapboxTelemetry] session.
+     * Disables metrics reporting and ends [EventsServiceInterface] and [TelemetryService] session.


Current implementation does not stop EventsService and TelemetryService.

What should this method do? If it must stop interaction and location events sending (for the entire application and all Mapbox SDKs we have in the application) we need to setEventsCollectionState(false) here.
If it should not stop events collection we need to remove Disables metrics reporting and ends [EventsServiceInterface] and [TelemetryService] session. from method description.

we need to setEventsCollectionState(false) here.

@tatiana-yan then it stops collecting metrics from every single SDK (nav, maps, search), right? It's not fit for the current method, let's change the description

Right, it will stop all except turnstiles for all SDKs. if you want to stop navigation events only, you can set some internal flag to ignore events sending
And is there some use-case when we need to stop collecting navigation events but not maps/search/locations? In that case, user should have independent settings to opt-out from maps/nav/search which looks unlikely.

I'm not sure that any customers use this API, but since it's public we should respect it. I'll add internal falgs

Actually, we're not recommending using this API at all. To align the API I'll add internal flags 👍

And is there some use-case when we need to stop collecting navigation events but not maps/search/locations?

don't think this must be done via MapboxReporter API

Capturing from a chat, @RingerJK to cut a ticket to discuss our recommendations on how developers/end-users should opt-out of telemetry collection (since TelemetryUtils#setEventsCollectionState is documented as internal). Not a blocker for this PR but has to be resolved before the changes go stable.

ref CORESDK-1357

libnavigation-base/src/main/java/com/mapbox/navigation/base/metrics/MetricsReporter.kt

...src/main/java/com/mapbox/navigation/core/telemetry/events/NavigationAppUserTurnstileEvent.kt

LukasPaczos · 2022-10-06T10:46:53Z

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

+            ioJobController.scope.launch {
+                metricsObserver?.onMetricUpdated(metricEvent.metricName, metricEvent.toJson(gson))
+            }


Why is this on a worker thread? I found the original PR in #2237 but it has no context. Documentation for the interface doesn't mention that it'll be called from a worker.

Not a blocker here, just something I noticed to discuss separately.

ref NAVAND-730

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

VysotskiVadim

I don't see any critical issues in code, but I can't do a proper review as I haven't undrstood yet how telemetry works 🙂

LukasPaczos

LGTM with a note that there are some open threads that need to be addressed before these changes go stable.

RingerJK requested a review from a team as a code owner September 30, 2022 17:32

RingerJK self-assigned this Sep 30, 2022

RingerJK added the skip changelog Should not be added into version changelog. label Sep 30, 2022

RingerJK changed the title ~~Kyv navand 647 core telemerty migration~~ Core telemetry migration Sep 30, 2022

RingerJK force-pushed the kyv-NAVAND-647-core-telemerty-migration branch 2 times, most recently from 3afb1c4 to 076ed39 Compare September 30, 2022 18:02

RingerJK added the blocked Blocked by dependency or unclarity. label Sep 30, 2022

RingerJK marked this pull request as draft October 3, 2022 06:26

dzinad reviewed Oct 3, 2022

View reviewed changes

Igor Tarasov and others added 7 commits October 3, 2022 23:46

Integration of Core Telemetry.

9e51eb1

publish_android_snapshot

Set tokens for MME from config for better events filtering

e2e6bf9

bump common sdk to 23.1.0-beta.2

d4111e9

- added tests; -fixes

57a89e5

wip

d985e72

ktlint fixes

b0d12b1

ktlint

c296c41

RingerJK force-pushed the kyv-NAVAND-647-core-telemerty-migration branch from 076ed39 to c296c41 Compare October 3, 2022 21:50

RingerJK removed the blocked Blocked by dependency or unclarity. label Oct 3, 2022

wip

fb95da0

RingerJK added 2 commits October 4, 2022 10:29

licence

86ac941

wip

f3195b5

RingerJK marked this pull request as ready for review October 4, 2022 21:57

RingerJK requested review from brunoabinader, tarigo and tatiana-yan October 4, 2022 21:57

wip

e8b0713

tatiana-yan reviewed Oct 5, 2022

View reviewed changes

RingerJK added the release blocker Needs to be resolved before the release. label Oct 5, 2022

dzinad reviewed Oct 5, 2022

View reviewed changes

wip

cfbc6dc

dzinad approved these changes Oct 5, 2022

View reviewed changes

RingerJK commented Oct 5, 2022

View reviewed changes

libnavigation-base/api/current.txt Outdated Show resolved Hide resolved

LukasPaczos reviewed Oct 5, 2022

View reviewed changes

RingerJK added 3 commits October 5, 2022 18:21

wip

7f7ba43

wip

0c5626d

revert removing mapbox-android-core

7000c78

RingerJK requested a review from LukasPaczos October 5, 2022 18:35

ktlint fixes

065f532

tatiana-yan reviewed Oct 6, 2022

View reviewed changes

RingerJK added 2 commits October 6, 2022 09:58

change docs

e1b38bb

MapboxMetricsReporter: handle disable nav telemerty

c3d71a6

LukasPaczos reviewed Oct 6, 2022

View reviewed changes

VysotskiVadim reviewed Oct 6, 2022

View reviewed changes

libnavigation-metrics/src/main/java/com/mapbox/navigation/metrics/MapboxMetricsReporter.kt Outdated Show resolved Hide resolved

VysotskiVadim approved these changes Oct 6, 2022

View reviewed changes

LukasPaczos approved these changes Oct 6, 2022

View reviewed changes

fix docs

2c9df75

tatiana-yan approved these changes Oct 6, 2022

View reviewed changes

RingerJK merged commit a2bf593 into main Oct 6, 2022

RingerJK deleted the kyv-NAVAND-647-core-telemerty-migration branch October 6, 2022 12:58

Guardiola31337 mentioned this pull request Oct 6, 2022

Initial Core Telemetry adoption. #5002

Closed

dzinad mentioned this pull request Oct 11, 2022

add INTERNET and ACCESS_NETWORK_STATE permissions #6473

Merged

Core telemetry migration #6423

Core telemetry migration #6423

Uh oh!

Conversation

RingerJK commented Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

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!

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!

tarigo Oct 6, 2022 • 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

RingerJK commented Sep 30, 2022 •

edited

Loading

codecov bot commented Oct 3, 2022 •

edited

Loading

tarigo Oct 6, 2022 •

edited

Loading