1527656 - Consider if `recordEvent` should ever throw

Status: RESOLVED FIXED

(Toolkit :: Telemetry, task, P1)

Description

[Chris H-C :chutten]

Histograms' accumulate functions don't throw.

Scalars' add/set/setmax functions don't throw.

Events' recordEvent throws, but only if the event is unknown.

Should we bring recordEvent into consistency with Histograms and Scalars? This bug is about evaluating users of recordEvent, specifically the ones that handle the exception thrown in the case that the event is unregistered.

Comment 1

[Chris H-C :chutten]

Of note might be how often recordEvent is called with an unknown event. We have data on that thanks to TELEMETRY_EVENT_RECORDING_ERROR. Here are the counts for beta and nightly (pre-release-only probe).

In short: it's happened maybe a hundred times ever on beta. And less than that on nightly.

Comment 2

[Georg Fritzsche [:gfritzsche]]

One use-case we should consider when changing things here:
There might be tests that want to know if recording an event succeeded (i.e. if it conformed to the schema of registered events, like bug 1443560).

Comment 3

[Georg Fritzsche [:gfritzsche]]

Hey Gijs, this is the bug where we're tracking the issue you mentioned.

Comment 4

[:Gijs (he/him)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #3)

Hey Gijs, this is the bug where we're tracking the issue you mentioned.

Thanks, I noticed because I got bugmail as I filed 1311100. :-)

Comment 5

[Ethan Glasser-Camp (:glasserc)]

My 2 cents: no, it shouldn't throw, and it definitely doesn't make sense for it to throw sometimes and silently fail other times (as when extra values are present that shouldn't be).

I think there are two use cases: one is during "real operation", where maybe we should never throw, and another is during testing, where I think we should throw. One way that we could maybe satisfy both is to for Telemetry to offer some kind of mock that calling code can use as a substitute for Telemetry in tests, and this mock should validate that events (and histograms, and scalars, etc.) are of the correct form, and this mock should throw when that assumption is violated. This would also let us write tests that don't rely on the current shared global state of the Telemetry object (i.e. getting a snapshot of events, having to clear events from other tests, etc.).

Comment 6

[:Gijs (he/him)]

We can use Cu.isInAutomation (and/or the C++ equivalent) to make the error behavior different between automation and what users see. We already make use of this e.g. in fluent's parsing/usage code.

Comment 7

[Georg Fritzsche [:gfritzsche]]

We're tracking this as P1 for now and will sort through the different event work & use-cases next week.

Comment 8

[Chris H-C :chutten]

To clarify:

• recordEvent should not throw in production, to match the Histogram and Scalar recording functions. We can and should log, though.
• recordEvent must throw in automation, to ensure tests as written can continue to be useful.

The current condition (recordEvent throwing even in production for the one type of failure that's under test) is acceptable for now, so let's push this to P2.

(( And a note for whoever picks this up: This might be complicated by some recordEvent validation not happening until it gets to the parent process, and by the C++ API which cannot throw. ))

Comment 9

[Georg Fritzsche [:gfritzsche]]

(In reply to Chris H-C :chutten from comment #8)

To clarify:

• recordEvent should not throw in production, to match the Histogram and Scalar recording functions. We can and should log, though.
• recordEvent must throw in automation, to ensure tests as written can continue to be useful.

+1 on the first point.

The automation problem might have other solutions, we should talk through that (make test harness fail on specific error logs, test specific mock, ...).

Having the same API throw only under some conditions is probably not a good pattern as it can be surprising.

Comment 10

[Chris H-C :chutten]

Attachment: Bug 1527656 - Telemetry.recordEvent should not throw r?janerik

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0b954c335af
Telemetry.recordEvent should not throw r=janerik

Comment 12

[Chris H-C :chutten]

Taking the follow-up of what to do in automation to bug 1542891. This bug is now only concerned with bringing recordEvent into semantic compliance with the rest of Telemetry's recording APIs.

Comment 13

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/e0b954c335af

Comment 14

[Hector Zhao [:hectorz]]

Hi,

Hi, it looks like a patch landed for this bug, but the doc at 1 still mentions

Throws if the combination of category, method and object is unknown.

and running

try {
  Services.telemetry.recordEvent("unknown_category", "unknown_method", "unknown_object");
  console.log("not throwing");
} catch (ex) {
  console.error(ex);
}

in browser console would throw.

Comment 15

[Chris H-C :chutten]

Huh, apparently JS_ReportErrorASCII doesn't just report errors, it throws them too. Thank you for catching this.

Comment 16

[Chris H-C :chutten]

Attachment: Bug 1527656 - Don't throw on recordEvent an unknown event r?janerik!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f936342d32cc
Don't throw on recordEvent an unknown event r=janerik

Comment 18

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/f936342d32cc