Consider if `recordEvent` should ever throw
Categories
(Toolkit :: Telemetry, task, P1)
Tracking
()
People
(Reporter: chutten, Assigned: chutten)
References
(Blocks 2 open bugs)
Details
Attachments
(2 files)
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.
Assignee | ||
Comment 1•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
Hey Gijs, this is the bug where we're tracking the issue you mentioned.
Comment 4•6 years ago
|
||
(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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
We're tracking this as P1 for now and will sort through the different event work & use-cases next week.
Assignee | ||
Comment 8•6 years ago
|
||
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•6 years ago
|
||
(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.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 10•6 years ago
|
||
Comment 11•6 years ago
|
||
Assignee | ||
Comment 12•6 years ago
|
||
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•6 years ago
|
||
bugherder |
Updated•6 years ago
|
Comment 14•5 years ago
|
||
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
andobject
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.
Assignee | ||
Comment 15•5 years ago
|
||
Huh, apparently JS_ReportErrorASCII
doesn't just report errors, it throws them too. Thank you for catching this.
Assignee | ||
Comment 16•5 years ago
|
||
Comment 17•5 years ago
|
||
Comment 18•5 years ago
|
||
bugherder |
Description
•