search

mapbox / mapbox-events-android

Mapbox telemetry and core libraries for Android.
https://docs.mapbox.com/android/core/overview
MIT License
60 stars 48 forks source link

MapboxTelemetry. updateDebugLoggingEnabled() setting could be ignored #382

Open osana opened 5 years ago

osana commented 5 years ago

The issue was noticed in Android Maps sdk TestApp. Follow the steps in this ticket to reproduce.

Looks like telemetryClient is lazily initialized in MapboxTelemetry and MapboxTelemetry.updateDebugLoggingEnabled() does not take that into account.

If MapboxTelemetry.updateDebugLoggingEnabled(loggingEnabled) is called before Mapbox.telemetryClient was initialzed, the loggingEnabled setting will be ignored

Guardiola31337 commented 5 years ago

We've also noticed this from the Navigation SDK.

As described in OP the TelemetryClient is now initialized lazily but clients can call updateDebugLoggingEnabled e.g. https://github.com/mapbox/mapbox-navigation-android/blob/ba2c4f1b4bae1443cd97a3edffb3338cc40959dd/libandroid-navigation/src/main/java/com/mapbox/services/android/navigation/v5/navigation/NavigationTelemetry.java#L128 before that 👀 https://github.com/mapbox/mapbox-events-android/blob/67b007f4265f32bf8db8ee3b78b2d467626e5a5e/libtelemetry/src/full/java/com/mapbox/android/telemetry/MapboxTelemetry.java#L146-L150 There's a null check to prevent crashes but the setting is not persisted.

Also noting that the TelemetryClient is not initialized until calling sendEvents https://github.com/mapbox/mapbox-events-android/blob/67b007f4265f32bf8db8ee3b78b2d467626e5a5e/libtelemetry/src/full/java/com/mapbox/android/telemetry/MapboxTelemetry.java#L302 for the first time.

cc @alfwatt @andrlee

andrlee commented 5 years ago

Also noting that the TelemetryClient is not initialized until calling sendEvents

@Guardiola31337 this is design intent ^. We should fix debug logging api, it should not change state of the client.

Guardiola31337 commented 5 years ago

@andrlee

We should fix debug logging api, it should not change state of the client.

In order to enable logging we need the state beforehand, it makes sense to not allowing changing it but we need to persist it somehow until the telemetry client is created so we can pass it through it or change completely how we're logging currently. Wondering how we'd support the use case in which a client would like to switch on and off logging 🤔 I mean with the current setup i.e. without changing the logging mechanism.

alfwatt commented 5 years ago

Prefs related (saved state not being persisted locally but saved in memory only)

Look at as part of Android config work.

Valid: https://github.com/mapbox/mobile-telemetry/issues/458

friendoye commented 3 years ago

Our team recently encountered same issue and we'd love to have ability to enable/disable logging explicitly during MapboxTelemetry initialization. There are several ways how to do it, so if we could discuss the optimal solution, then, I believe, it will be easier to create PR with requested behavior.