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

RESOLVED FIXED

Status

P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: thills, Assigned: thills)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [spark])

Attachments

(4 attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Assignee: nobody → thills
Whiteboard: [spark]
Blocks: 1161650
Priority: -- → P1
(Assignee)

Comment 1

3 years ago
Created attachment 8604173 [details] [diff] [review]
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
Attachment #8604173 - Flags: feedback?(marshall)
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) {
  // ...
});
Attachment #8604173 - Flags: feedback?(marshall)
(Assignee)

Comment 3

3 years ago
Created attachment 8605878 [details] [diff] [review]
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
Flags: needinfo?(marshall)
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Flags: needinfo?(marshall)
(Assignee)

Comment 4

3 years ago
Created attachment 8606665 [details] [diff] [review]
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
Attachment #8606665 - Flags: feedback?(marshall)
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..
Attachment #8606665 - Flags: feedback?(marshall) → feedback+

Comment 6

3 years ago
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.
Tamara, do you have any updates on this bug?
Flags: needinfo?(thills)
(Assignee)

Comment 8

3 years ago
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
Flags: needinfo?(thills) → needinfo?(marshall)
Created attachment 8612350 [details] [review]
[gaia] tamarahills:bugfix/1160483-imei-dogfood > mozilla-b2g:master
(Assignee)

Updated

3 years ago
Flags: needinfo?(marshall)
(Assignee)

Comment 10

3 years ago
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
Attachment #8612350 - Flags: review?(marshall)
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..
Attachment #8612350 - Flags: review?(marshall) → review-
Summary: [Metrics] Replace the deviceID field in FTU and AppUsage with IMEI for a special build flag → [Metrics] Replace the deviceID field in AppUsage with IMEI for a special build flag
See Also: → bug 1169349
(Assignee)

Comment 12

3 years ago
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 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.
Attachment #8612350 - Flags: review- → review+
You need to log in before you can comment on or make changes to this bug.