1831294 - Background update failing to log reasons in local builds - unnecessary console output?

Status: NEW

(Toolkit :: Application Update, defect, P3)

Description

[Mark Banner (:standard8)]

I had started up my local build of Firefox, and after about 5 minutes of running, I got these messages repeated on the console 11 times:

11:45:02.411 Metric had 1 error(s) of type invalid_value!
11:45:02.411 Error recording reasonsToNotUpdate BackgroundUpdate.sys.mjs:847:19

The first one is flagged as an error, the second is a standard log.

The logging has a couple of issues.

Firstly it is not clear as to if this is an issue that needs to be addressed, or if it is simply a warning/logging. The real error isn't logged, and it isn't logging the values that failed to be logged.

Secondly, the code is logging twice firstly to the background update logger as a debug statement, then to the real console as a log statement.

Maybe the console.log is a left-over statement? If it is, I think that given the call to testGetValue generates an error to the console, which doesn't contain useful information for debug, we should probably keep an additional information message to the console that the previous message has been generated and may be ignored (?) because of the appropriate reason.

Ideally, we'd use something other than testGetValue, so no errors would be logged if we're not interested in them, however, from previous comments it doesn't look like that is available, but maybe that should be filed.

Comment 1

[Chris H-C :chutten]

Oh goodness, testGetValue should only be used in test contexts. My comment suggesting its use was me assuming :nalexander was asking for test reasons. My apologies for not reading the context more closely and following the patch through its evolution.

Please remove uses of testGetValue from non-test code.

The intention of limits like string_list's is that you keep recording to that metric and if any invalid_overflow happens, that signifies that the collection is incomplete in the ping containing it.

Comment 2

[Nick Alexander :nalexander [he/him]]

This is really on me: I thought the use was test-only as well and lost track of it as the code evolved. I'll work with Max to address this.