+++ This bug was initially created as a clone of Bug #1315906 +++ For calls like: > Services.telemetry.scalarSetMaximum("some_uint_scalar", "not a number"); ... we should warn, but not throw an exception.

Comment on attachment 8827442 [details] [diff] [review] bug1321790.patch Chris, do you think this is a reasonable approach/refactoring for the API? This POC changes only JS API function (Add), factoring most of the code out in a way that can be reused across all the other API endpoints. It also changes the API so that it doesn't throw anymore, but rather prints to the console through |internal_LogScalarError| (that will be updated with the error messages). Georg is out right now, so flagging you for feedback!

Comment on attachment 8827442 [details] [diff] [review] bug1321790.patch Review of attachment 8827442 [details] [diff] [review]: ----------------------------------------------------------------- Seems legit. It seems wrong to me to return NS_OK when it isn't actually okay, it's more like "Yeah, we handled that case. It wasn't exceptional", but I guess returning anything not NS_OK gets turned into a thrown error in js? ::: toolkit/components/telemetry/TelemetryScalar.cpp @@ +1053,5 @@ > return scalar; > } > > +/** > + * Empty comment @@ +1089,5 @@ > + // Don't throw on expired scalars. > + if (rv == NS_ERROR_NOT_AVAILABLE) { > + return ScalarResult::Ok; > + } > + return ScalarResult::UnknownScalar; Is this for sure because the scalar is unknown, or is this an unknown error? If the latter, should probably be called UnknownError or similar

(In reply to Chris H-C :chutten from comment #3) > Comment on attachment 8827442 [details] [diff] [review] > bug1321790.patch > > Review of attachment 8827442 [details] [diff] [review]: > ----------------------------------------------------------------- > > Seems legit. It seems wrong to me to return NS_OK when it isn't actually > okay, it's more like "Yeah, we handled that case. It wasn't exceptional", > but I guess returning anything not NS_OK gets turned into a thrown error in > js? Wow, that was quick! Yes, returning anything other than NS_OK makes the function throw (we are returning NS_ERROR_* without this patch). > @@ +1089,5 @@ > > + // Don't throw on expired scalars. > > + if (rv == NS_ERROR_NOT_AVAILABLE) { > > + return ScalarResult::Ok; > > + } > > + return ScalarResult::UnknownScalar; > > Is this for sure because the scalar is unknown, or is this an unknown error? > If the latter, should probably be called UnknownError or similar It's because the scalar is unknown.

Chris, here's the complete patch. Could you review it if you have any cycle?

Comment on attachment 8827890 [details] [diff] [review] bug1321790.patch Review of attachment 8827890 [details] [diff] [review]: ----------------------------------------------------------------- Everything I see is landable. One comment nit, and if we _can_ tell in tests that something was logged, we probably should check that. ::: toolkit/components/telemetry/TelemetryScalar.cpp @@ +1188,5 @@ > + * Update the keyed scalar with the provided value. This is used by the JS API. > + * > + * @param aName The scalar name. > + * @param aKey The key name. > + * @param aType The action type for updating scalars across processes. It's not just used for the cross-process case, is it? ::: toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js @@ +88,5 @@ > Telemetry.clearScalars(); > > + // The JS API must not throw when used incorrectly but rather print > + // a message to the console. > + Telemetry.scalarAdd(NON_EXISTING_SCALAR, 11715); Should we assert that something was logged? (can we tell that?)

(In reply to Chris H-C :chutten from comment #6) > Comment on attachment 8827890 [details] [diff] [review] > bug1321790.patch > > Review of attachment 8827890 [details] [diff] [review]: > ----------------------------------------------------------------- > > Everything I see is landable. One comment nit, and if we _can_ tell in tests > that something was logged, we probably should check that. There's bug 1324774 filed for making tests fail on Telemetry warnings. We can make that bug also test that telemetry warnings are actually printed in test_TelemetryScalars.js and other internal Telemetry tests.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/904be9728888 Change the JS Scalar API so that it doesn't throw. r=chutten