Closed Bug 1321790 Opened 3 years ago Closed 3 years ago

JS scalar add functions should not throw

Categories

(Toolkit :: Telemetry, defect, P1)

defect
Points:
2

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)

+++ 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.
Priority: P3 → P2
Priority: P2 → P1
Points: --- → 2
Assignee: nobody → alessio.placitelli
Attached patch bug1321790.patch (obsolete) — Splinter Review
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 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+
(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.
Attached patch bug1321790.patch (obsolete) — Splinter Review
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)
Status: NEW → ASSIGNED
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+
(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.
Attached patch bug1321790.patchSplinter Review
Attachment #8827890 - Attachment is obsolete: true
Attachment #8827914 - Flags: review+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/904be9728888
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.