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

Include mobile country code in Analytics events #226

Merged
merged 5 commits into from Nov 7, 2019

Conversation

@bemasc
Copy link
Contributor

bemasc commented Nov 1, 2019

This should reduce country reporting errors in Firebase.
This change also reorganizes event reporting to reduce
boilerplate.

This should reduce country reporting errors in Firebase.
This change also reorganizes event reporting to reduce
boilerplate.
@bemasc bemasc requested review from fortuna and alalamav Nov 1, 2019
DOWNLOAD,
DURATION,
LATENCY,
MCC,

This comment has been minimized.

Copy link
@fortuna

fortuna Nov 1, 2019

We need to be clearer about what MCC you are referring to: the connected network or the home network? Let's make that clear in the names.

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 4, 2019

Author Contributor

I changed the code to only report when they agree, to minimize confusion.

if (mcc != null) {
return mcc;
}
int gsmMCC = context.getResources().getConfiguration().mcc;

This comment has been minimized.

Copy link
@fortuna

fortuna Nov 1, 2019

Is this the recommended way, versus using the Telephony api?

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 4, 2019

Author Contributor

I switched to the Telephony API, in order to get both SIM and Network codes.

if (mcc != null) {
return mcc;
}
int gsmMCC = context.getResources().getConfiguration().mcc;

This comment has been minimized.

Copy link
@fortuna

fortuna Nov 1, 2019

It seems the logic for getting the MCC from a context is quite application-independent, so I recommend moving it out of the application code. I guess that would be its own class. Or at least make it a static method that makes it clear it doesn't need the AnalyticsEvent state.

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 4, 2019

Author Contributor

OK, I've moved it into a new class.

// Get the system's mobile country code, or 0 if it could not be determined.
// After the first call to this function, all subsequent calls will return immediately.
private int getMCC() {
if (mcc != null) {

This comment has been minimized.

Copy link
@fortuna

fortuna Nov 1, 2019

I suggest you pass the home MCC in the constructor instead, and provide a factory function that takes the context. It will make the class a lot simpler, more testable and more reusable.

This comment has been minimized.

Copy link
@fortuna

fortuna Nov 1, 2019

Is there any benefit of lazy initiation?

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 4, 2019

Author Contributor

I've moved the context into a factory method as you suggested.

bytesEvent.putInt(Names.DURATION.name(), summary.getDuration());
FirebaseAnalytics.getInstance(vpnService).logEvent(Names.BYTES.name(), bytesEvent);
new AnalyticsEvent(vpnService)
.put(Params.UPLOAD, summary.getUploadBytes())

This comment has been minimized.

Copy link
@fortuna

fortuna Nov 1, 2019

I'm wondering if it makes sense to create factory functions or static methods like SendBytesEvent(upload_bytes, download_bytes, port, tcpHandshakeMs, duration)

That would hide all the Params details from the application code and provide some type safety. Also, most of the time you set the params and send in the same call.

Even though it may result in a little more code, you'll be isolating complexity and the application code will be more readable and simpler (e.g. 1 line instead of 7 to send an event).

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 4, 2019

Author Contributor

Done!

bemasc added 2 commits Nov 4, 2019
This change also moves Firebase-related classes into their
own package, to distinguish them from the general sys classes.
@bemasc bemasc force-pushed the bemasc-mcc branch from a54fe92 to ce8cd02 Nov 4, 2019
if (simMCC != null) {
// Require the SIM and network country codes to match before reporting a country, to avoid
// confusion when they disagree (e.g. international roaming).
Integer networkMCC = extractMCC(telephonyManager.getNetworkOperator());

This comment has been minimized.

Copy link
@fortuna

fortuna Nov 6, 2019

Use telephonyManager.getNetworkCountryIso() instead. No need for extractMCC here

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 6, 2019

Author Contributor

Done

Class<?> systemPropertiesClass = Class.forName("android.os.SystemProperties");
Method get = systemPropertiesClass.getMethod("get", String.class);
String cdmaOperator = (String)get.invoke(systemPropertiesClass,
"ro.cdma.home.operator.numeric");

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 6, 2019

Author Contributor

Thanks. I think that doesn't affect this code.

Class<?> systemPropertiesClass = Class.forName("android.os.SystemProperties");
Method get = systemPropertiesClass.getMethod("get", String.class);
String cdmaOperator = (String)get.invoke(systemPropertiesClass,
"ro.cdma.home.operator.numeric");

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 6, 2019

Author Contributor

That constant is also only accessible by reflection, and is not guaranteed to be stable, so I think we're better off using the setting's name.

}
TelephonyManager telephonyManager =
(TelephonyManager)context.getSystemService(Context.TELEPHONY_SERVICE);
Integer simMCC = extractMCC(telephonyManager.getSimOperator());

This comment has been minimized.

Copy link
@fortuna

fortuna Nov 6, 2019

It seems this may work for CDMA too. I see gsm.sim.operator.numeric being set for CDMA here: https://android.googlesource.com/platform/frameworks/opt/telephony/+/ddeba83b6c611b3c408df12ae6de8a22a4311047/src/java/com/android/internal/telephony/cdma/CDMAPhone.java#209

Still worth it having the fallback though.

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 6, 2019

Author Contributor

It looks like that was added in ~2016, so it should work for newer devices.

}
TelephonyManager telephonyManager =
(TelephonyManager)context.getSystemService(Context.TELEPHONY_SERVICE);
Integer simMCC = extractMCC(telephonyManager.getSimOperator());

This comment has been minimized.

Copy link
@fortuna

fortuna Nov 6, 2019

Use telephonyManager.getSumCountryIso() instead. No need for extractMCC.

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 6, 2019

Author Contributor

Done

* Gets the system's mobile country code, or 0 if it could not be determined.
* After the first call to this function, all subsequent calls will return immediately.
*/
static int get(Context context) {

This comment has been minimized.

Copy link
@fortuna

fortuna Nov 6, 2019

Let's return the country iso? It seems to be doable, and a lot better than MCC. See the iso methods below.

There's also MccTable.countryCodeForMcc(): https://android.googlesource.com/platform/frameworks/opt/telephony/+/master/src/java/com/android/internal/telephony/MccTable.java

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 6, 2019

Author Contributor

Done

class MobileCountryCode {
// Mobile Country Code, or 0 if it could not be determined. This is used to memoize get()
// across all the analytics events.
private static Integer mcc = null;

This comment has been minimized.

Copy link
@fortuna

fortuna Nov 6, 2019

Let's move the memoization to the AnalyticsWrapper instead, since this is higher level logic that can cause confusion at a low level (really an application optimization)

The logic of only using the country if they match could go in the AnalyticsWrapper too, since that's clearly application logic.

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 6, 2019

Author Contributor

Done

// Mobile Country Code, or 0 if it is unknown.
private final int mcc;
private final FirebaseAnalytics analytics;
private AnalyticsWrapper(int mcc, FirebaseAnalytics analytics) {

This comment has been minimized.

Copy link
@fortuna

fortuna Nov 6, 2019

I think we should move to ISO country codes instead, since MCC is a pain to convert.

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 6, 2019

Author Contributor

Done

}

private void log(Events e, @NonNull BundleBuilder b) {
b.put(Params.MCC, mcc);

This comment has been minimized.

Copy link
@fortuna

fortuna Nov 6, 2019

We need a name here that conveys:

  • It's an ISO country code, not an MCC
  • It's the home operator and network operator, set when the user is connected to its home cell network

It would also be useful to better understand when those things are set. I see 3 states:

  • user on cell network at home
  • user on cell network while roaming
  • user not on cell network (e.g. wifi)

What do you think about this:

  • CELL_HOME_COUNTRY: the "sim operator country". Set if the phone has a SIM card or cdma configured
  • CELL_NETWORK_COUNTRY (maybe CELL_CONNECTED_COUNTRY? CELL_NET_COUNTRY?): Empty if not available (not on a cell network), set to the country if available and == CELL_HOME_COUNTRY or set to ZZ if it differs from CELL_HOME_COUNTRY, for privacy

This way we'll be able to better understand when those things are set, and the meaning of the parameters will be clearer.

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 6, 2019

Author Contributor

I went with COUNTRY (the consensus country, or empty string if there is none) and NETWORK (the network type). I think that's the minimal solution that gives us the necessary information and lets us exclude outlier events.

This change also adds reporting of the connection type.
/**
* @return The consensus country code, or "" if there isn't one.
*/
@NonNull String getCombinedCountry() {

This comment has been minimized.

Copy link
@fortuna

fortuna Nov 6, 2019

I'm ok with providing a combined country, provided we know which country we are using.
We need to better understand the data before trusting it. We need to see how those country parameters perform in the wild.

An alternative to my previous proposal is to add a new parameter that tells which country you picked. For example: COUNTRY_DEVICE, COUNTRY_NETWORK, COUNTRY_DEVICE_AND_NETWORK.

Though I have the impression the combined country will make it harder to pick different combinations. I suggested reporting the two locations instead (with the home or network country anonymized in case of roaming), because it's easy to apply filters like device_country == network_country. Or ip_country == network_country.

This comment has been minimized.

Copy link
@bemasc

bemasc Nov 7, 2019

Author Contributor

OK, done.

@fortuna
fortuna approved these changes Nov 7, 2019
@bemasc bemasc merged commit 1675a87 into master Nov 7, 2019
3 checks passed
3 checks passed
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@bemasc bemasc deleted the bemasc-mcc branch Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.