1324774 - Make sure that Telemetry console warnings/errors triggers automation failures

Status: NEW

(Toolkit :: Telemetry, defect, P3)

Description

[Alessio Placitelli [:Dexter]]

Scalar, histogram and event recording are (or soon will) only printing warning/errors to the console if someone tries to record invalid data. No exception is thrown.

We should catch these errors in the test harness and make tests fail.

Comment 1

[Alessio Placitelli [:Dexter]]

I received some good pointers on this by :standard8 (thanks!).

Devtools are doing something similar [1] by registering a console listener and formatting their messages in a different way for other tools to pick up. We could follow the same route and throw exceptions instead.

Hello did something similar [2] (but in content!), and so did Thunderbird [3].

[1] - https://dxr.mozilla.org/mozilla-central/rev/7083c0d30e75fc102c715887af9faec933e936f8/devtools/shared/tests/unit/head_devtools.js#19
[2] - https://github.com/mozilla/loop/blob/master/shared/test/loop_mocha_utils.js#L267
[3] - https://dxr.mozilla.org/comm-central/rev/b6764a038123881a4422370fb6e8ead6bf7f0e73/mailnews/db/gloda/test/unit/resources/glodaTestHelper.js#102

Comment 3

[Georg Fritzsche [:gfritzsche]]

We should have this for xpcshell, mochitest, marionette (in rough order of priority).

Comment 4

[:Gijs (he/him)]

I was about to file a dupe of this, specifically:

When looking at bug 1659193 I noticed it had some JS errors in the console that looked unrelated to the thing we're investigating there, specifically: "Not a string" - without a stack or any other information.

It so happens I just got a pernosco recording of that intermittent and so it was rather easy to work out what was triggering those errors. It turned out they come from https://searchfox.org/mozilla-central/rev/8d108a59d067ce37671090b0b1972ee8adfb7196/toolkit/components/telemetry/core/TelemetryHistogram.cpp#2189 where adding a keyed histogram with a non-string key throws an error. But the error has no meaningful information and the console error object created doesn't include stack info, so from just logs it's impossible to know what is causing the error.

I think on debug builds and/or when running automated tests (ie using xpc::IsInAutomation()), these things should assert. Then we'll get stacks and it'll be much easier to work out what is going wrong, and we can ensure we don't hit these error conditions in automated tests, which will help ensure our telemetry etc. works the way it's expected to.

Alessio, I don't suppose you have time to work on this...? (I'd set a needinfo but you've got them blocked...)

Comment 5

[Alessio Placitelli [:Dexter]]

Redirecting to Chris, as sadly I can't work on this right now :(

Comment 6

[Chris H-C :chutten]

Hooooo, okay.

I suppose we could, in LogToBrowserConsole add a quick

if (aLogLevel == nsIScriptError::Error && xpc::IsInAutomation()) {
  MOZ_ASSERT(false, aMsg);
}

But I don't know what the ramifications would be at this point in Telemetry's maturity. I guess we could pick a day a little ways from a merge day and see what previously-successful tests intermittently or perma fail on CI. I don't think I'll have the time to work on this in a helpful timeframe, though.