1160483 - [Metrics] Replace the deviceID field in AppUsage with IMEI for a special build flag

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect, P1)

Description

[Tamara Hills [:thills]]

For Spark/Lightsaber users, we want to have a special build flag that replaces the UUIDs generated for both the FTU and AppUsage Pings with the IMEI.

This is for the dogfooding purposes.

The IMEI should be available via the telephony interfaces.

Comment 1

[Tamara Hills [:thills]]

Attachment: dogfoodimei.diff

Hi Marshall,

Looking for your thoughts on this direction for this patch.  I'm not that happy with it because I wanted to put the entire thing into telemetry.js but ran into some race conditions there.

Thanks,

-tamara

Comment 2

[Marshall Culpepper [:marshall_law]]

Comment on attachment 8604173 [details] [diff] [review]
dogfoodimei.diff

Review of attachment 8604173 [details] [diff] [review]:
-----------------------------------------------------------------

Hrm, yeah it would definitely be preferable for the device ID generation code to be shared moving forward. What kind of race conditions are you running into?

::: apps/system/js/ftu_ping.js
@@ +185,5 @@
> +    getImeiCode: function fp_getImeiCode() {
> +    var dialPromise = navigator.mozTelephony.dial('*#06#', 0);
> +
> +      return dialPromise.then(function about_dialImeiPromise(call) {
> +        return call.result.then(function(result) {

I'm pretty sure you can chain then(...) on a top level promise by returning the promise directly, so instead of nesting, you can do..

return dialPromise.then(function(call) {
  return call.promise;
}).then(function(result) {
  // ...
});

Comment 3

[Tamara Hills [:thills]]

Attachment: promisesproblem.diff

Hey Marshall,

Do you think you can take a look at this approach?  What's happening when I put it all into telemetry.js is the promise is that setting the args.deviceID to the imei does not seem to work.  The var self = this trick does not seem to work.

I also tried just setting args.deviceID and then letting everything else be done outside of the last .then, but then it seems that TelemetryRequest.send gets called first and it just uses the old deviceID.

I'm sure I'm missing something really basic.  Do you mind having a look?  It seems to get the IMEI just fine.

Thanks,

-tamara

Comment 4

[Tamara Hills [:thills]]

Attachment: sparkimei.diff

Hi Marshall,

I wound up putting this in the send per your advice.  This helped a little bit, but I ran into some scoping issues with imei variable.  I solved this by having everything in the prom.then.  Also, I put the conditional check for whether it's a dogfooder or not inside the first promise and resolved it immediately if it's not a dogfooder (so we don't have to call the imei code in this case).  

Let me know what you think of this approach.

THanks,

-tamara

Comment 5

[Marshall Culpepper [:marshall_law]]

Comment on attachment 8606665 [details] [diff] [review]
sparkimei.diff

Review of attachment 8606665 [details] [diff] [review]:
-----------------------------------------------------------------

This looks mostly good, see my comments :)

::: shared/js/telemetry.js
@@ +70,5 @@
> +    var self = this;
> +    var prom = new Promise(function(resolve, reject) {
> +      if(!TR.DOGFOOD) {
> +        console.log("resolving with: " + self.packet.info.deviceID);
> +        resolve(self.packet.info.deviceID)

I think you meant |self.info.deviceID| here? You also use it below

@@ +79,5 @@
> +        }).then(function(result) {
> +          return result.statusMessage;
> +        }).then(function (imei) {
> +          debug('IMEI IS: ' + imei);
> +          resolve(imei);

Unless |result.statusMessage| is a Promise, I think this last callback isn't really necessary. It may be better to just |resolve(result.statusMessage)| in the previous callback..

Comment 6

[Jean Gong :jgong]

In talking to Tamara about this particular bug, it take a bit of time to do the testing since there is a feedback loop that takes time to run and validate.  The ETA for to get this done is May 29th.

Comment 7

[Doug Sherk (:drs) (inactive)]

Tamara, do you have any updates on this bug?

Comment 8

[Tamara Hills [:thills]]

Hi Doug,

I'm pretty badly stuck on the unit test for this change right now.  I will NI to Marshall on this and see if I can get some advice.  If I can't find a way around this in short order, I may have to go back to the original approach (which isn't the best as there is some duplication).

Thanks,

-tamara

Comment 9

[GitHub Autolander Bot]

Attachment: [gaia] tamarahills:bugfix/1160483-imei-dogfood > mozilla-b2g:master

Comment 10

[Tamara Hills [:thills]]

Comment on attachment 8612350 [details] [review]
[gaia] tamarahills:bugfix/1160483-imei-dogfood > mozilla-b2g:master

Hi Marshall,

This patch is just localized to the AUM.  I know we wanted to try and consolidate everything into Telemetry module, but I didn't see a way to implement that with major code changes in the AUM.  I thought it will introduce to much risk for a dogfooding feature.

Let me know your thoughts as this needs to be landed soon.

Thanks,
-tamara

Comment 11

[Marshall Culpepper [:marshall_law]]

Comment on attachment 8612350 [details] [review]
[gaia] tamarahills:bugfix/1160483-imei-dogfood > mozilla-b2g:master

Too bad we couldn't get this working in the shared Telemetry code, but that isn't a blocker IMO. Are you going to have another patch for FTU?

This approach looks fine, but I'm worried we aren't dealing with Promise.reject() properly when trying to get the IMEI. There's also a fair amount of code cleanup needed before this can land..

r+ once those are addressed..

Comment 12

[Tamara Hills [:thills]]

Hi Jean/Doug, Still working on this.  Reworked things a bit in the code with help from Marshall.  I'm working on a set of tests right now.  Hoping to have something for review later tonight for Marshall to look over.

Comment 13

[Marshall Culpepper [:marshall_law]]

Comment on attachment 8612350 [details] [review]
[gaia] tamarahills:bugfix/1160483-imei-dogfood > mozilla-b2g:master

Newest AUM dogfooding code looks much better, r+ while Tamara waits on the testing results.

Comment 14

[Tamara Hills [:thills]]

commit: https://github.com/mozilla-b2g/gaia/commit/1428c18b748addc56abaa559dec914a41550df07
try run: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=6984f941c9d607f58afe205acf11208aae4bb326