Closed Bug 1458363 Opened 2 years ago Closed 2 years ago

Remove WEAVE_ENGINE_SYNC_ERRORS histogram

Categories

(Firefox :: Sync, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: standard8, Assigned: markh)

References

Details

Attachments

(1 file)

In bug 1458235, I'm looking into cases where we do things like `Assert.ok(foo, 1)`, when we meant to check they were equal.

I've just found an issue in test_errorhandler_2.js:

https://searchfox.org/mozilla-central/rev/795575287259a370d00518098472eaa5b362bfa7/services/sync/tests/unit/test_errorhandler_2.js#411-412

```
  let syncErrors = sumHistogram("WEAVE_ENGINE_SYNC_ERRORS", { key: "catapult" });
  Assert.ok(syncErrors, 1);
```

If I change that to `Assert.equals` then the test fails with:

FAIL test_sync_engine_generic_fail - [test_sync_engine_generic_fail : 412] 2 == 1

At a quick glance, I can't tell what is meant to be the correct error count here.

Can someone take a look please?

(btw, this was introduced in bug 1210296 afaict).
Flags: needinfo?(markh)
Flags: needinfo?(kit)
Mark/Thom, do you remember if we still use `WEAVE_ENGINE_SYNC_ERRORS`? IIUC, all our dashboards use the Sync ping's richer error reporting...
Thanks Mark! I've verified that the reason it fails with the .equal instead of the erroneous .ok is due to previous tests causing this histogram to get a value. A fix for that is quite trivial (just reset the histogram count at the start of that test) but...

(In reply to Kit Cambridge (they/them) [:kitcambridge] from comment #1)
> Mark/Thom, do you remember if we still use `WEAVE_ENGINE_SYNC_ERRORS`? IIUC,
> all our dashboards use the Sync ping's richer error reporting...

Yeah - I don't think this histogram is adding any value now we have a reliable sync ping, so the correct thing to do is probably to nuke it - so I'll change this but into one doing exactly that.
Flags: needinfo?(markh)
Flags: needinfo?(kit)
Summary: Sync's test_errorhandler_2.js has incorrect check for number of logged histogram errors → Remove WEAVE_ENGINE_SYNC_ERRORS histogram
Assignee: nobody → markh
Status: NEW → ASSIGNED
Comment on attachment 8972484 [details]
Bug 1458363 - Remove WEAVE_ENGINE_SYNC_ERRORS histogram.

https://reviewboard.mozilla.org/r/241074/#review246874

Woo!
Attachment #8972484 - Flags: review?(kit) → review+
Comment on attachment 8972484 [details]
Bug 1458363 - Remove WEAVE_ENGINE_SYNC_ERRORS histogram.

Chris, can you take a look?
Attachment #8972484 - Flags: review?(gfritzsche) → review?(chutten)
Comment on attachment 8972484 [details]
Bug 1458363 - Remove WEAVE_ENGINE_SYNC_ERRORS histogram.

https://reviewboard.mozilla.org/r/241074/#review247232

Incredibly pleased to see histograms cleaned up when they've proven unuseful!
Attachment #8972484 - Flags: review?(chutten) → review+
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/54bd7eb9c5ef
Remove WEAVE_ENGINE_SYNC_ERRORS histogram. r=chutten,kitcambridge
https://hg.mozilla.org/mozilla-central/rev/54bd7eb9c5ef
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.