Skip to content

Don't send events if telemetry is disabled #6512

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Oct 27, 2022
Merged

Don't send events if telemetry is disabled #6512

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7521e3f
don't send events if telemetry is disabled
VysotskiVadim Oct 26, 2022
3a32b09
log event's names instead of full events
VysotskiVadim Oct 26, 2022
cba98ab
always initialize and deinitialize telemetry
VysotskiVadim Oct 26, 2022
2d31b2a
added changelog
VysotskiVadim Oct 26, 2022
d27578e
don't log if telemetry is turned off
VysotskiVadim Oct 26, 2022
1497a1e
rename refactoring
VysotskiVadim Oct 27, 2022
8113a69
fixed grammar
VysotskiVadim Oct 27, 2022
f6b6626
Revert "always initialize and deinitialize telemetry"
VysotskiVadim Oct 27, 2022
000cefc
updated changelog
VysotskiVadim Oct 27, 2022
fa03557
Update CHANGELOG.md
VysotskiVadim Oct 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ Mapbox welcomes participation and contributions from everyone.
#### Bug fixes and improvements
- Improved experience in tunnels and reduced the likelihood of losing road edge matching candidates. [#6510](https://github.com/mapbox/mapbox-navigation-android/pull/6510)
- Fixed an issue with `NavigationView` that caused road label position to not update in some cases. [#6508](https://github.com/mapbox/mapbox-navigation-android/pull/6508)
- Fixed an issue when the SDK were trying to send telemetry events when it's when telemetry is switched off globally. [#6512](https://github.com/mapbox/mapbox-navigation-android/pull/6512)
- Fixed an issue when the SDK didn't stop telemetry when mapbox navigation is destroyed. [#6512](https://github.com/mapbox/mapbox-navigation-android/pull/6512)

## Mapbox Navigation SDK 2.9.0-beta.3 - 21 October, 2022
### Changelog
Expand Down
6 changes: 6 additions & 0 deletions libnavigation-base/src/main/java/com/mapbox/navigation/base/internal/metric/ValueUtil.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package com.mapbox.navigation.base.internal.metric

import com.mapbox.bindgen.Value

fun Value.extractEventsNames(): List<String>? = (this.contents as? List<Value>)
?.mapNotNull { (it.contents as? Map<String, Value>)?.get("event")?.contents as? String }
83 changes: 31 additions & 52 deletions libnavigation-core/src/main/java/com/mapbox/navigation/core/MapboxNavigation.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ import com.mapbox.navigation.core.trip.session.eh.GraphAccessor
import com.mapbox.navigation.core.trip.session.eh.RoadObjectMatcher
import com.mapbox.navigation.core.trip.session.eh.RoadObjectsStore
import com.mapbox.navigation.metrics.MapboxMetricsReporter
import com.mapbox.navigation.metrics.internal.TelemetryUtilsDelegate
import com.mapbox.navigation.navigator.internal.MapboxNativeNavigator
import com.mapbox.navigation.navigator.internal.NavigatorLoader
import com.mapbox.navigation.navigator.internal.router.RouterInterfaceAdapter
Expand Down Expand Up @@ -506,23 +505,21 @@ class MapboxNavigation @VisibleForTesting internal constructor(
)

ifNonNull(accessToken) { token ->
runInTelemetryContext { telemetry ->
logD(
"MapboxMetricsReporter.init from MapboxNavigation main",
telemetry.LOG_CATEGORY
)
MapboxMetricsReporter.init(
navigationOptions.applicationContext,
token,
obtainUserAgent()
)
MapboxMetricsReporter.toggleLogging(navigationOptions.isDebugLoggingEnabled)
telemetry.initialize(
this,
navigationOptions,
MapboxMetricsReporter,
)
}
logD(
"MapboxMetricsReporter.init from MapboxNavigation main",
MapboxNavigationTelemetry.LOG_CATEGORY
)
MapboxMetricsReporter.init(
Copy link
Contributor

@dzinad dzinad Oct 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see the corresponding addition of flag check in MapboxMetricsReporter. Is this intentional?
Same for toggleLogging and disable.

Copy link
Contributor Author

@VysotskiVadim VysotskiVadim Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, didn't get this one

Copy link

@LukasPaczos LukasPaczos Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's okay in this context. We shouldn't if-check on init because someone could flip the global setting back at any point in time. And the TelemetryUtils.setEventsCollectionState also disables the telemetry service under the hood so we don't need to check for that.

navigationOptions.applicationContext,
token,
obtainUserAgent()
)
MapboxMetricsReporter.toggleLogging(navigationOptions.isDebugLoggingEnabled)
MapboxNavigationTelemetry.initialize(
Copy link

@LukasPaczos LukasPaczos Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the processing in MapboxNavigationTelemetry would now be running even if telemetry collection is disabled. Could we improve that control to avoid the unnecessary processing?

Copy link
Contributor Author

@VysotskiVadim VysotskiVadim Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's a fair point. I see following options:

1. Keep as is

pros:

  • telemetry events are sent whenever the SDK client turns off and on telemetry globally
  • simple code

cons:

  • MapboxNavigationTelemetry works but events aren't sent when telemetry is disabled

2. Initiaize MapboxNavigationTelemetry only when telemetry is turned on, always disable on destroy

pros:

  • MapboxNavigationTelemetry doesn't work when telemetry is disabled globally
  • simple code

cons:

  • telemetry events won't be sent if telemetry was turned off globally during initialization and then turned on in runtime

3. keep current telemetry state (enabled/disabled) on the sdk side, observe it and start/stop MapboxNavigationTelemetry

pros:

  • telemetry events are sent whenever the SDK client turns off and on telemetry globally
  • MapboxNavigationTelemetry doesn't work when telemetry is disabled globally

cons:

  • complex code

I proffer the option 1

It doesn't seem that MapboxNavigationTelemetry consumes a lot of resources while working, so I think option 1 is a reasonable tradeoff for us. @LukasPaczos , WDYT?

Copy link

@LukasPaczos LukasPaczos Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be in favor of option 2 that matches the existing behavior (avoid processing) and we already have NAVAND-355 that tracks the runtime enable/disable changes which we can tackle separately.

Copy link
Contributor Author

@VysotskiVadim VysotskiVadim Oct 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let's keep current telemetry initialisation as is and improve it in NAVAND-355. Reverted cba98ab

this,
navigationOptions,
MapboxMetricsReporter,
)
}

val routeOptionsProvider = RouteOptionsUpdater()
Expand Down Expand Up @@ -1047,9 +1044,7 @@ class MapboxNavigation @VisibleForTesting internal constructor(
historyRecordingStateHandler.unregisterAllStateChangeObservers()
historyRecordingStateHandler.unregisterAllCopilotSessionObservers()
developerMetadataAggregator.unregisterAllObservers()
runInTelemetryContext { telemetry ->
telemetry.destroy(this@MapboxNavigation)
}
MapboxNavigationTelemetry.destroy(this@MapboxNavigation)
threadController.cancelAllNonUICoroutines()
threadController.cancelAllUICoroutines()
ifNonNull(reachabilityObserverId) {
Expand Down Expand Up @@ -1455,17 +1450,15 @@ class MapboxNavigation @VisibleForTesting internal constructor(
feedbackMetadata: FeedbackMetadata?,
userFeedbackCallback: UserFeedbackCallback?,
) {
runInTelemetryContext { telemetry ->
telemetry.postUserFeedback(
feedbackType,
description,
feedbackSource,
screenshot,
feedbackSubType,
feedbackMetadata,
userFeedbackCallback,
)
}
MapboxNavigationTelemetry.postUserFeedback(
feedbackType,
description,
feedbackSource,
screenshot,
feedbackSubType,
feedbackMetadata,
userFeedbackCallback,
)
}

@ExperimentalPreviewMapboxNavigationAPI
Expand All @@ -1474,13 +1467,11 @@ class MapboxNavigation @VisibleForTesting internal constructor(
@CustomEvent.Type customEventType: String,
customEventVersion: String,
) {
runInTelemetryContext { telemetry ->
telemetry.postCustomEvent(
payload = payload,
customEventType = customEventType,
customEventVersion = customEventVersion
)
}
MapboxNavigationTelemetry.postCustomEvent(
payload = payload,
customEventType = customEventType,
customEventVersion = customEventVersion
)
}

/**
Expand All @@ -1493,11 +1484,7 @@ class MapboxNavigation @VisibleForTesting internal constructor(
*/
@ExperimentalPreviewMapboxNavigationAPI
fun provideFeedbackMetadataWrapper(): FeedbackMetadataWrapper =
runInTelemetryContext { telemetry ->
telemetry.provideFeedbackMetadataWrapper()
} ?: throw java.lang.IllegalStateException(
"To get FeedbackMetadataWrapper Telemetry must be enabled"
)
MapboxNavigationTelemetry.provideFeedbackMetadataWrapper()

/**
* Start observing alternatives routes for a trip session via [RouteAlternativesObserver].
Expand Down Expand Up @@ -1773,14 +1760,6 @@ class MapboxNavigation @VisibleForTesting internal constructor(
}
}

private inline fun <T> runInTelemetryContext(func: (MapboxNavigationTelemetry) -> T): T? {
return if (TelemetryUtilsDelegate.getEventsCollectionState()) {
func(MapboxNavigationTelemetry)
} else {
null
}
}

private fun obtainUserAgent(): String {
return "$MAPBOX_NAVIGATION_USER_AGENT_BASE/${BuildConfig.MAPBOX_NAVIGATION_VERSION_NAME}"
}
Expand Down
25 changes: 0 additions & 25 deletions libnavigation-core/src/test/java/com/mapbox/navigation/core/MapboxNavigationTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,19 +339,6 @@ internal class MapboxNavigationTest : MapboxNavigationBaseTest() {
verify(exactly = 1) { navigationSession.unregisterAllNavigationSessionStateObservers() }
}

@Test
fun telemetryIsDisabled() {
every { TelemetryUtilsDelegate.getEventsCollectionState() } returns false

createMapboxNavigation()
mapboxNavigation.onDestroy()

verify(exactly = 0) {
MapboxNavigationTelemetry.initialize(any(), any(), any(), any())
}
verify(exactly = 0) { MapboxNavigationTelemetry.destroy(any()) }
}

@ExperimentalPreviewMapboxNavigationAPI
@Test(expected = IllegalStateException::class)
fun telemetryIsDisabledTryToGetFeedbackMetadataWrapper() {
Expand Down Expand Up @@ -1630,18 +1617,6 @@ internal class MapboxNavigationTest : MapboxNavigationBaseTest() {
verify(exactly = 1) { MapboxNavigationTelemetry.postCustomEvent(any(), any(), any()) }
}

@Test
fun `when telemetry is disabled custom event is not posted`() = coroutineRule.runBlockingTest {
createMapboxNavigation()
every { TelemetryUtilsDelegate.getEventsCollectionState() } returns false
every { MapboxNavigationTelemetry.postCustomEvent(any(), any(), any()) } just Runs
every { MapboxNavigationTelemetry.destroy(any()) } just Runs

mapboxNavigation.postCustomEvent("", NavigationCustomEventType.ANALYTICS, "1.0")

verify(exactly = 0) { MapboxNavigationTelemetry.postCustomEvent(any(), any(), any()) }
}

@Test
fun requestRoadGraphDataUpdate() {
val callback = mockk<RoadGraphDataUpdateCallback>()
Expand Down
18 changes: 14 additions & 4 deletions libnavigation-metrics/src/main/java/com/mapbox/navigation/metrics/MapboxMetricsReporter.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,13 @@ import com.mapbox.common.EventsServiceObserver
import com.mapbox.common.TelemetryService
import com.mapbox.common.TurnstileEvent
import com.mapbox.navigation.base.internal.metric.MetricEventInternal
import com.mapbox.navigation.base.internal.metric.extractEventsNames
import com.mapbox.navigation.base.metrics.MetricEvent
import com.mapbox.navigation.base.metrics.MetricsObserver
import com.mapbox.navigation.base.metrics.MetricsReporter
import com.mapbox.navigation.metrics.internal.EventsServiceProvider
import com.mapbox.navigation.metrics.internal.TelemetryServiceProvider
import com.mapbox.navigation.metrics.internal.TelemetryUtilsDelegate
import com.mapbox.navigation.utils.internal.InternalJobControlFactory
import com.mapbox.navigation.utils.internal.logD
import com.mapbox.navigation.utils.internal.logE
Expand Down Expand Up @@ -44,11 +46,19 @@ object MapboxMetricsReporter : MetricsReporter {
private val eventsServiceObserver =
object : EventsServiceObserver {
override fun didEncounterError(error: EventsServiceError, events: Value) {
logE("EventsService failure: $error for event $events", LOG_CATEGORY)
ifTelemetryIsRunning {
logE(LOG_CATEGORY) {
"EventsService failure: $error for events ${events.extractEventsNames()}"
}
}
}

override fun didSendEvents(events: Value) {
logD("Event has been sent $events", LOG_CATEGORY)
ifTelemetryIsRunning {
logD(LOG_CATEGORY) {
"Events has been sent ${events.extractEventsNames()}"
}
}
}
}

Expand Down Expand Up @@ -150,10 +160,10 @@ object MapboxMetricsReporter : MetricsReporter {
}

private inline fun ifTelemetryIsRunning(func: () -> Unit) {
if (enableTelemetry) {
if (enableTelemetry && TelemetryUtilsDelegate.getEventsCollectionState()) {
func.invoke()
} else {
logW(
logD(
"Navigation Telemetry is disabled",
LOG_CATEGORY
)
Expand Down
17 changes: 14 additions & 3 deletions ...vigation-metrics/src/test/java/com/mapbox/navigation/metrics/MapboxMetricsReporterTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import com.mapbox.navigation.testing.LoggingFrontendTestRule
import com.mapbox.navigation.testing.MainCoroutineRule
import com.mapbox.navigation.utils.internal.InternalJobControlFactory
import com.mapbox.navigation.utils.internal.LoggerFrontend
import com.mapbox.navigation.utils.internal.logW
import io.mockk.every
import io.mockk.just
import io.mockk.mockk
Expand Down Expand Up @@ -53,7 +52,7 @@ class MapboxMetricsReporterTest {
mockkObject(TelemetryServiceProvider)

every { TelemetryUtilsDelegate.setEventsCollectionState(any()) } just runs
every { TelemetryUtilsDelegate.getEventsCollectionState() } returns false
every { TelemetryUtilsDelegate.getEventsCollectionState() } returns true
}

@After
Expand All @@ -76,7 +75,7 @@ class MapboxMetricsReporterTest {
MapboxMetricsReporter.addEvent(mockk())

verify(exactly = 2) {
logger.logW(
logger.logD(
"Navigation Telemetry is disabled",
"MapboxMetricsReporter"
)
Expand All @@ -96,6 +95,18 @@ class MapboxMetricsReporterTest {
verify(exactly = 0) { eventService.sendEvent(any(), any()) }
}

@Test
fun `events aren't sent if telemetry is disabled globally`() {
val eventService = initMetricsReporterWithTelemetry()

every { TelemetryUtilsDelegate.getEventsCollectionState() } returns false
MapboxMetricsReporter.sendTurnstileEvent(mockk())
MapboxMetricsReporter.addEvent(mockk())

verify(exactly = 0) { eventService.sendTurnstileEvent(any(), any()) }
verify(exactly = 0) { eventService.sendEvent(any(), any()) }
}

@Test
fun telemetryPushCalledWhenAddValidEvent() = coroutineRule.runBlockingTest {
mockkObject(InternalJobControlFactory) {
Expand Down