1151267 - app update telemetry for UPDATE_CANNOT_APPLY_* is reversed

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Robert Strong (they/them - no direct email)]

The telemetry reports true when the user can apply updates.

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch rev1

Comment 2

[Robert Strong (they/them - no direct email)]

Comment on attachment 8589905 [details] [diff] [review]
patch rev1

try push
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac9cf2a1d972

Comment 3

[Stephen A Pohl [:spohl]]

Comment on attachment 8589905 [details] [diff] [review]
patch rev1

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

Looks good, just one question before I can r+ this.

::: toolkit/mozapps/update/nsUpdateService.js
@@ -1439,5 @@
>          break;
>        case STATE_PENDING:
>          stateCode = 4;
> -        parts[0] = STATE_FAILED;
> -        parts.push(STAGE_FAIL_FALLBACK);

Was this meant to be removed in this patch? If so, could you give me some context why we want to remove this here?

Comment 4

[Robert Strong (they/them - no direct email)]

Because the state code of STATE_PENDING provides the same info already. I need to come up with a way to report the specific error which I'll do in another bug

Comment 5

[Robert Strong (they/them - no direct email)]

Does my reply in comment #4 make sense?

Comment 6

[Stephen A Pohl [:spohl]]

Is it safe to assume that not having parts[0] set to STATE_FAILED will not cause other incorrect behavior? For example, if parts[0] is not set to STATE_FAILED in refreshUpdateStatus, we don't seem to call handleFallbackToCompleteUpdate [1]. Do we care?

[1]  http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#3349

Comment 7

[Robert Strong (they/them - no direct email)]

(In reply to Stephen Pohl [:spohl] from comment #6)
> Is it safe to assume that not having parts[0] set to STATE_FAILED will not
> cause other incorrect behavior? For example, if parts[0] is not set to
> STATE_FAILED in refreshUpdateStatus, we don't seem to call
> handleFallbackToCompleteUpdate [1]. Do we care?
> 
> [1] 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/
> nsUpdateService.js#3349
The only place this is changed is in pingStateAndStatusCodes, the only thing this change affects is the telemetry ping, and this is intentional. This makes it so it doesn't report the "custom" error of 97 when the state is STATE_PENDING when calling pingStatusErrorCode. 

Of course we care and in this case the change is the right thing to do. It isn't something we had at some point in the past.

Can you point to some code where this change might cause some problem?

Comment 8

[Stephen A Pohl [:spohl]]

Comment on attachment 8589905 [details] [diff] [review]
patch rev1

Thanks, this makes sense now!

Comment 9

[Robert Strong (they/them - no direct email)]

Pushed to fx-team
https://hg.mozilla.org/integration/fx-team/rev/5949612d030a

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5949612d030a

Comment 11

[Robert Strong (they/them - no direct email)]

Comment on attachment 8589905 [details] [diff] [review]
patch rev1

Approval Request Comment
[Feature/regressing bug #]: Bug 1137447
[User impact if declined]: It will be more difficult to analyze telemetry for app update which helps to identify areas where we need to improve.
[Describe test coverage new/current, TreeHerder]: None
[Risks and why]: Little to none. Tested locally. The code wraps the calls in try blocks to prevent it from breaking app update. It has also been on m-c.
[String/UUID change made/needed]: None

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8589905 [details] [diff] [review]
patch rev1

Approving for uplift to aurora. This has had a day on m-c and seems low risk.

Comment 13

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-aurora
https://hg.mozilla.org/releases/mozilla-aurora/rev/7f06e1d0445d