Add locale metric to metrics ping

[brizental]

Fixes Bug 1601489

This is how this will look in the metrics ping:

   {
      "client_info": {
        "android_sdk_version": "28",
        "app_build": "0",
        "app_display_version": "glean.version.name",
        "architecture": "armeabi-v7a",
        "client_id": "41dabd38-1b93-473b-8bc1-0e4284a65c53",
        "device_manufacturer": "unknown",
        "device_model": "robolectric",
        "first_run_date": "2020-01-09+01:00",
        "os": "Android",
        "os_version": "9",
        "telemetry_sdk_build": "23.0.1"
      },
      "metrics": {
        "string": {
          "glean.baseline.locale": "en-US"
        }
      },
      "ping_info": {
        "end_time": "2020-01-09T17:24+01:00",
        "ping_type": "metrics",
        "seq": 0,
        "start_time": "2020-01-09T17:24+01:00"
      }
    }

[brizental]

Hm, the tests failing here are tests such as sending an empty ping doesn't queue work and that is happening, because now Glean adds the locale to the metrics ping and even if the client doesn't add any metrics there, there will always be the locale there and so it never will be empty.

I could of course add an exception and say that if there is only locale in the metrics ping that means it is still empty, but that is not the prettiest solution. Second option would be to circle back and reconsider adding locale to the client_info.

I am sure that are more options that I haven't thought of. Ideas?

[mdboom]

Just commenting. Obviously the code changes are fine (with the exception of the test failure you already pointed out).

The name "glean.baseline.locale" is a bit unfortunate. We could instead make a new metric "l10n.locale" or something, but then it would exist in different names in the baseline and metrics pings. Lots of suboptimal options here... Not sure of the best way forward.

[travis79]

I'm not sure I like having this trigger the metrics ping to always be sent, but I'm less of a fan of special code to handle one special metric to prevent it. If that's the choices then my vote is to put this in client_info, which this does seem like client_info material...

[badboy]

The naming issue is indeed unfortunate. I defer a decision on that to Dexter.
Always triggering a metrics ping doesn't sound too bad to me, I expect there to be mostly always other metric data in there (totally unproven though, I could be totally wrong and we're holding back sending a lot of empty metrics pings).

Re the test: that one can be fixed by using an adhoc-created custom ping, that's definitely gonna be empty then.

[Dexterp37]

The name "glean.baseline.locale" is a bit unfortunate. We could instead make a new metric "l10n.locale" or something, but then it would exist in different names in the baseline and metrics pings. Lots of suboptimal options here... Not sure of the best way forward.

Yeah :\ I wish we named it better back then :( We have a few options.

1. Rename the glean.baseline.locale to l10n.locale and call it a breaking change. (let's not!)
2. Add l10n.locale and mark glean.baseline.locale as deprecated in the changelog. Then remove this a the end of the quarter. We should send a PSA email to fx-data-dev announcing this.

I'd lean towards 2.

With respect to "always" sending the 'metrics' ping, this makes it no worse than the current state, already: GeckoView introduced a 'user' lifetime metric that will always make the 'metrics' ping available...

[codecov-io]

Codecov Report

Merging #633 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #633      +/-   ##
============================================
+ Coverage     74.65%   74.66%   +<.01%     
  Complexity      265      265              
============================================
  Files            96       96              
  Lines          5942     5944       +2     
  Branches        731      731              
============================================
+ Hits           4436     4438       +2     
  Misses          971      971              
  Partials        535      535

Impacted Files	Coverage Δ	Complexity Δ
...oid/src/main/java/mozilla/telemetry/glean/Glean.kt	88.1% <100%> (+0.06%)	1 <0> (ø)	⬇️
glean-core/ios/Glean/Glean.swift	91.52% <100%> (+0.04%)	0 <0> (ø)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82cbdb5...019f090. Read the comment docs.

[brizental]

After discussions in IRC, we came to the conclusion that the best option was to add this to the client_info. This is the case because then we don't always send the metrics ping. Always sending it turns out to change way more behaviour than expected.

All tests will fail here, because this changes need to be added to mozilla-pipeline-schemas first. I opened a PR for that already: mozilla-services/mozilla-pipeline-schemas#482

I thought it would be best to leave the locale in the baseline ping body as well for now, even though it is redundant, so we can do as suggested be @Dexterp37 and remove that later.

[Dexterp37]

All tests will fail here, because this changes need to be added to mozilla-pipeline-schemas first. I opened a PR for that already: mozilla-services/mozilla-pipeline-schemas#482

One trick here that can let you check your tests is to change this line to point to your mozilla-pipeline-schma branch ;-)

I thought it would be best to leave the locale in the baseline ping body as well for now, even though it is redundant, so we can do as suggested be @Dexterp37 and remove that later.

That's fine. Please do file a bug about removing that.

[brizental]

One trick here that can let you check your tests is to change this line to point to your mozilla-pipeline-schma branch ;-)

I've done this locally and all tests pass :)

One more thing, just for illustration purposes, this is what the new client_info will look like. I have some manual testing locally too:

"client_info": {
  "android_sdk_version": "25",
  "app_build": "1",
  "app_display_version": "1.0",
  "architecture": "x86_64",
  "client_id": "43359b2d-30f5-4bdf-a2f6-cbc956bcfa9a",
  "device_manufacturer": "unknown",
  "device_model": "Android SDK built for x86_64",
  "first_run_date": "2020-01-15+00:00",
  "locale": "en-US",
  "os": "Android",
  "os_version": "7.1.1",
  "telemetry_sdk_build": "24.0.0"
}

[Dexterp37]

Please add an entry to the changelog file.

Additionally, please do file a bug about removing the glean.baseline.locale metric.

[brizental]

Now blocked by: https://bugzilla.mozilla.org/show_bug.cgi?id=1609892

[brizental]

Unblocked by 64ea5bf :) If all turns out green in CI I will just add the changes to the changelog and this should be ready for final review.

[mdboom]

\o/

[mdboom]

This looks right to me -- this is the current master of MPS, which has the locale field in it.