Dynamic telemetry initialisation/deinitialisation based on user preferences by DzmitryFomchyn · Pull Request #7755 · mapbox/mapbox-navigation-android

DzmitryFomchyn · 2024-02-22T09:31:10Z

Description

https://mapbox.atlassian.net/browse/NAVAND-1795

Fixed a crash that can happen when user dynamically changes telemetry collection preferences.

…rence

github-actions · 2024-02-22T09:31:38Z

Changelog

Features

Added billing explanation logs. Now Navigation SDK explains in the logs why certain Active Guidance or Free Drive Trip session started/stopped/paused/resumed. Billing explanations have [BillingExplanation] prefix in the logcat. [#7710](https://github.com/mapbox/mapbox-navigation-android/pull/7710)

Bug fixes and improvements

Fixed leak of CarAppLifecycleOwner on every copilot start. [#7669](https://github.com/mapbox/mapbox-navigation-android/pull/7669)
Resolved an issue where a crash could occur if telemetry sending settings were changed after creating MapboxNavigation. [#7755](https://github.com/mapbox/mapbox-navigation-android/pull/7755)

Known issues ⚠️

Other changes

Added Polish translation for UI elements.

Android Auto Changelog

Features

Bug fixes and improvements

The app is now considered as the one in active navigation only when an active route is set to MapboxNavigation. Previously it was always considered active. [#7366](https://github.com/mapbox/mapbox-navigation-android/pull/7366)
When Android Auto host tells the app to stop active navigation because another app starts navigating, now SDK will enter FreeDrive mode instead of stopping trip session completely. [#7366](https://github.com/mapbox/mapbox-navigation-android/pull/7366)

dzinad · 2024-02-22T15:01:19Z

+        navigationOptions: NavigationOptions,
+        userAgent: String
+    ) {
+        check(!isWrapperInitialized) {


Do we really want to crash here?

I don't have a strong opinion on this.

If we just return here (without crashing) in case of repeated initialisation, we might skip another issue which caused doubled initialisation. At the same time I don't think the current solution is the best.

I think we could crash only for debug builds, but this practice is not common in our project now. WDYT about the following approach?

if (isWrapperInitialized) { if (BuildConfig.DEBUG) { error("Already initialized") } else { return } }

Yes, this is nice, but maybe let's not if it, but introduce an utility assert method that will only crash in debug. By the way, isn't there already something like that?

By the way, isn't there already something like that?

I haven't found anything like that. I've added check and failing only in debug builds

dzinad · 2024-02-22T15:01:25Z

+    }
+
+    fun destroy() {
+        check(isWrapperInitialized) {


Do we really want to crash here?

Let's discuss it here and I'll fix this place as well, if case of need.

dzinad · 2024-02-22T15:05:39Z

+    private var isTelemetryEnabled = false
+
+    private val telemetryStateObserver = TelemetryStateWatcher.Observer { isEnabled ->
+        if (!isTelemetryEnabled && isEnabled) {


Don't we also have to take isWrapperInitialized flag into account? I mean we shouldn't invoke initializeSdkTelemetry if we hadn't invoked initialize beforehand, right?

It's not obvious but isTelemetryEnabled can be true only if TelemetryWrapper is initialised. Similary, TelemetryStateWatcher.Observer can be triggered at all only if class is initialised.

I don't like this much, but given that it's incapsulated in one class, it seems to be ok.

codecov · 2024-02-22T15:16:16Z

Codecov Report

Attention: Patch coverage is 73.40426% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 74.15%. Comparing base (9e5e7ce) to head (0804ae7).

@@             Coverage Diff              @@
##               main    #7755      +/-   ##
============================================
+ Coverage     74.12%   74.15%   +0.03%     
- Complexity     6252     6263      +11     
============================================
  Files           852      855       +3     
  Lines         33705    33757      +52     
  Branches       4012     4021       +9     
============================================
+ Hits          24983    25033      +50     
+ Misses         7172     7166       -6     
- Partials       1550     1558       +8

Files	Coverage Δ
...gation/core/telemetry/MapboxNavigationTelemetry.kt	`80.89% <ø> (ø)`
...m/mapbox/navigation/core/telemetry/SdkTelemetry.kt	`100.00% <100.00%> (ø)`
...mapbox/navigation/core/MapboxNavigationProvider.kt	`45.00% <77.77%> (-0.46%)`	⬇️
...com/mapbox/navigation/utils/internal/Assertions.kt	`0.00% <0.00%> (ø)`
...ava/com/mapbox/navigation/core/MapboxNavigation.kt	`72.44% <76.00%> (+2.12%)`	⬆️
...pbox/navigation/core/telemetry/TelemetryWrapper.kt	`74.50% <74.50%> (ø)`

dzinad · 2024-02-23T08:18:57Z

        return mapboxNavigation!!
    }

+    @VisibleForTesting


Why do you need this?

This is needed in order to provide mocked TelemetryWrapper for unit tests.

dzinad · 2024-02-23T13:00:31Z

+        this.userAgent = userAgent
+
+        if (TelemetryUtilsDelegate.getEventsCollectionState()) {
+            initializeSdkTelemetry()


Hmm, what if someone disables the telemetry at runtime? Should we stop sending events? It's not supported now, right?

Yes, right, nothing changes here comparing to older SDK versions. It's not that easy to solve the issue you described beucase we might end up with incomplete telemetry events (for example, departure event witout arrival). We'll have to solve this later.

…rences (#7755) * Dynamic telemetry initialisation/deinitialisation based on user preference * Fix tests * TODO issue; Changelog * Fail only in debug mode * Don't observe telemetry state changes for now * Address PR comments; Add tests

* Dynamic telemetry initialisation/deinitialisation based on user preferences (#7755) * Dynamic telemetry initialisation/deinitialisation based on user preference * Fix tests * TODO issue; Changelog * Fail only in debug mode * Don't observe telemetry state changes for now * Address PR comments; Add tests * Rename changelog file

Dynamic telemetry initialisation/deinitialisation based on user prefe…

793bb72

…rence

DzmitryFomchyn requested a review from a team as a code owner February 22, 2024 09:31

DzmitryFomchyn added 2 commits February 22, 2024 15:33

Fix tests

3818077

TODO issue; Changelog

bbc149c

dzinad reviewed Feb 22, 2024

View reviewed changes

DzmitryFomchyn requested a review from dzinad February 22, 2024 15:37

DzmitryFomchyn added 2 commits February 22, 2024 17:44

Fail only in debug mode

60b444b

Don't observe telemetry state changes for now

ab337f5

DzmitryFomchyn force-pushed the df-dynamic-telemetry-initialisation branch from e7d193a to ab337f5 Compare February 22, 2024 20:36

dzinad reviewed Feb 23, 2024

View reviewed changes

DzmitryFomchyn force-pushed the df-dynamic-telemetry-initialisation branch from 7860fdb to 8ec962f Compare February 23, 2024 11:15

DzmitryFomchyn requested a review from dzinad February 23, 2024 11:15

Address PR comments; Add tests

0804ae7

DzmitryFomchyn force-pushed the df-dynamic-telemetry-initialisation branch from 8ec962f to 0804ae7 Compare February 23, 2024 11:48

dzinad reviewed Feb 23, 2024

View reviewed changes

dzinad approved these changes Feb 23, 2024

View reviewed changes

DzmitryFomchyn merged commit 00bc5e7 into main Feb 23, 2024

DzmitryFomchyn deleted the df-dynamic-telemetry-initialisation branch February 23, 2024 14:11

DzmitryFomchyn mentioned this pull request Feb 23, 2024

Telemetry fix backport #7756

Closed

DzmitryFomchyn mentioned this pull request Feb 23, 2024

Telemetry fix backport #7757

Merged

Uh oh!

Conversation

DzmitryFomchyn commented Feb 22, 2024

Description

Uh oh!

github-actions Bot commented Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Features

Bug fixes and improvements

Known issues ⚠️

Other changes

Features

Bug fixes and improvements

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!

DzmitryFomchyn Feb 22, 2024 • 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!

DzmitryFomchyn Feb 22, 2024 • 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!

Uh oh!

codecov Bot commented Feb 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Feb 22, 2024 •

edited

Loading

DzmitryFomchyn Feb 22, 2024 •

edited

Loading

DzmitryFomchyn Feb 22, 2024 •

edited

Loading

codecov Bot commented Feb 22, 2024 •

edited

Loading