Various Sync xpcshell tests are going to permafail when Gecko is bumped to version 49

RESOLVED FIXED

Status

()

defect
--
critical
RESOLVED FIXED
3 years ago
9 months ago

People

(Reporter: RyanVM, Assigned: markh)

Tracking

unspecified
Points:
---
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

3 years ago
Merge day is about two weeks away, so this is pretty critical.

https://treeherder.mozilla.org/logviewer.html#?job_id=18720292&repo=try
Flags: needinfo?(markh)
Assignee

Comment 1

3 years ago
As seems customary these days, most of these errors are due to telemetry probes expiring.
Flags: firefox-backlog+
Assignee

Comment 2

3 years ago
Lots of probes are set to expire in 48, so this removes them completely (ie, from Histograms.json, from the code and from the tests).

Ryan, can you please verify this solves the test failures? The all still pass locally for me, but I'm not sure how to push a try run that simulates a world post-merge.
Assignee: nobody → markh
Status: NEW → ASSIGNED
Flags: needinfo?(markh) → needinfo?(ryanvm)
Attachment #8737044 - Flags: review?(kcambridge)
Reporter

Comment 3

3 years ago
https://hg.mozilla.org/try/rev/75ce8e268e62 is all you need to apply locally.
Flags: needinfo?(ryanvm)
Comment on attachment 8737044 [details] [diff] [review]
0001-Bug-1261181-remove-telemetry-probes-set-to-expire-in.patch

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

Looks good! <3 deleting superfluous code.

::: services/sync/modules/engines.js
@@ +1228,5 @@
>      Observers.notify("weave:engine:sync:applied", count, this.name);
>    },
>  
>    _noteApplyFailure: function () {
> +    // here would be a good place to record telemetry...

The probes we had weren't actionable, I assume.
Attachment #8737044 - Flags: review?(kcambridge) → review+
Assignee

Comment 6

3 years ago
(In reply to Kit Cambridge [:kitcambridge] from comment #5)
> The probes we had weren't actionable, I assume.

Probably closer to saying "they weren't being actioned" (but yeah, last I looked we actioned all we could).

Note that even without this change, the probes were being ignored starting 49, so there's no value in keeping them without bumping the expires version. 48 will be in release until August this year, so the data is still available, and I figured we should just re-add new probes as we find them necessary.
Cool, sounds like a plan!
Reporter

Comment 8

3 years ago
Comment on attachment 8737044 [details] [diff] [review]
0001-Bug-1261181-remove-telemetry-probes-set-to-expire-in.patch

Looks good on Try, thanks!
Attachment #8737044 - Flags: feedback+
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eaab4a335361
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.