Skip to content
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.

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

Obfuscation problem with AppUserTurnstile event #456

Closed
yunikkk opened this issue Feb 20, 2020 · 9 comments
Closed

Obfuscation problem with AppUserTurnstile event #456

yunikkk opened this issue Feb 20, 2020 · 9 comments
Assignees
Labels

Comments

@yunikkk
Copy link
Contributor

yunikkk commented Feb 20, 2020

Configuration

  • SDK Version - 4.7.0

Steps to Reproduce
We've been facing an issue with MAU calculation from Chinese customer (Sogou) for some time.
Turned out, telemetry library does not expose proguard config to keep AppUserTurnstile from obfuscation, in fact none of the classes from com.mapbox.android.telemetry package are being kept via proguard config.

So if user of some library (eg. Vision), which in turn uses Telemetry is not explicitly keeping telemetry classes from proguard, AppUserTurnstile will be obfuscated, and then converted to JSON with wrong schema, eg.

[
  {
    "c": "appUserTurnstile",
    "d": "2020-02-20T18:58:22.326+0300",
    "e": "cbff5455-344f-42ba-96a7-6c542ca3255a",
    "enabled.telemetry": true,
    "g": "B2N_sprout",
    "h": "MapboxVision",
    "i": "0.12.0",
    "j": "Nokia 7 plus",
    "k": "Android - 10",
    "l": null
  }
]

Such JSON will then fail to be processed on server so user won't be calculated as MAU.

BTW, you can notice that "enabled.telemetry" is not renamed due to explicit annotation that is used by gson @SerializedName("enabled.telemetry").

Several other mapbox libraries contain specific Telemetry related proguard exclusion configs, eg. https://github.com/mapbox/mapbox-navigation-android/blob/master/libandroid-navigation/proguard-consumer.pro. So users on Navigation won't have issue with MAU calculation.

At the same time I haven't found same config at Maps proguard so I wonder if it's possible that maps users may have similar issues that we have in Vision with missing MAU when obfuscation is turned on?

Also I've not checked further but it seems other Events may have similar issues with gson serialization.

Expected
Telemetry sends events with correct schema even if obfuscation is turned on

cc @kiryldz

@yunikkk yunikkk added the bug label Feb 20, 2020
@alfwatt
Copy link

alfwatt commented Feb 20, 2020

Looks like we need:

  • update to the proguard config to stop our symbols from getting mangled
  • integration test which runs the code through any popular obfuscators our customers are using
  • update to the documentation to make a note about obfuscation issues

@yunikkk
Copy link
Contributor Author

yunikkk commented Feb 21, 2020

@mapbox/maps-android could anyone please check that Maps actually keeps telemetry classes from obfuscation at the moment with current telem releases?

@tobrun
Copy link
Member

tobrun commented Feb 21, 2020

I don't think that is the case. @Chaoba can you run point on fixing this for mapbox-gl-native-android?

image

@alfwatt
Copy link

alfwatt commented Feb 24, 2020

@mapbox/maps-android can ya'll make a copy of this issue for the parallel work in Maps?

We should also invite @mapbox/navigation to this obscure party to make sure we have integration tests for all the SDKs that cover these obfuscation issues.

@zugaldia
Copy link
Member

zugaldia commented Mar 2, 2020

@mapbox/maps-android can ya'll make a copy of this issue for the parallel work in Maps?

This was addressed by mapbox/mapbox-gl-native-android#194 (thanks @Chaoba!)

@tobrun
Copy link
Member

tobrun commented Mar 3, 2020

Small update for map side of things. We have a 9.1.0-alpha.1 version out with latest events library + proguard config on our end. We won't be backporting events library to older version but will include a proguard config to make sure we aren't obfuscating telemetry. We are planning a 9.0.1 and 8.7.3 with these changes.

@alfwatt
Copy link

alfwatt commented Mar 30, 2020

@yunikkk please verify that these changes are working for you in the latest version of the Telemetry SDK and close the ticket if you believe we've addressed the issue completely

@alfwatt alfwatt closed this as completed Mar 30, 2020
@alfwatt alfwatt reopened this Mar 30, 2020
@yunikkk
Copy link
Contributor Author

yunikkk commented Mar 31, 2020

@alfwatt we've shipped new build with the updated telemetry inside and can confirm fix is working! Closing the issue

@yunikkk yunikkk closed this as completed Mar 31, 2020
@Guardiola31337
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants