Closed
Bug 1321790
Opened 8 years ago
Closed 8 years ago
JS scalar add functions should not throw
Categories
(Toolkit :: Telemetry, defect, P1)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: gfritzsche, Assigned: Dexter)
References
(Blocks 1 open bug)
Details
(Whiteboard: [measurement:client])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Dexter
:
review+
|
Details | Diff | Splinter Review |
+++ 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.
Assignee | ||
Updated•8 years ago
|
Priority: P3 → P2
Reporter | ||
Updated•8 years ago
|
Priority: P2 → P1
Reporter | ||
Updated•8 years ago
|
Points: --- → 2
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → alessio.placitelli
Assignee | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
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!
Attachment #8827442 -
Flags: feedback?(chutten)
Comment 3•8 years ago
|
||
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
Attachment #8827442 -
Flags: feedback?(chutten) → feedback+
Assignee | ||
Comment 4•8 years ago
|
||
(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.
Assignee | ||
Comment 5•8 years ago
|
||
Chris, here's the complete patch. Could you review it if you have any cycle?
Attachment #8827442 -
Attachment is obsolete: true
Attachment #8827890 -
Flags: review?(chutten)
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Comment 6•8 years ago
|
||
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?)
Attachment #8827890 -
Flags: review?(chutten) → review+
Assignee | ||
Comment 7•8 years ago
|
||
(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.
Assignee | ||
Comment 8•8 years ago
|
||
Attachment #8827890 -
Attachment is obsolete: true
Attachment #8827914 -
Flags: review+
Assignee | ||
Comment 9•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 10•8 years ago
|
||
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
Keywords: checkin-needed
Comment 11•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in
before you can comment on or make changes to this bug.
Description
•