Cannot use telemetry.setEventRecordingEnabled to disable a dynamic telemetry category
Categories
(Toolkit :: Telemetry, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox-esr68 | --- | wontfix |
firefox68 | --- | wontfix |
firefox69 | --- | fixed |
People
(Reporter: robwu, Assigned: chutten)
References
(Regression)
Details
(Keywords: regression)
Attachments
(1 file)
It seems not possible to disable the recording of a dynamic telemetry event. This was mentioned in a patch to bug 1460400, but the limitation is not documented (at least, not in events.rst).
Open the global JS console and run:
var cat = "dyncat";
Services.telemetry.registerEvents(cat, {met: {methods:["met"],objects:["obj"],record_on_release:true} });
Services.telemetry.setEventRecordingEnabled(cat, false);
Services.telemetry.recordEvent(cat, "met", "obj");
console.log(Services.telemetry.snapshotEvents(Ci.nsITelemetry.DATASET_ALL_CHANNELS, false).dynamic.filter(e => e[1] === cat));
Expected:
- The result should be an empty array.
Actual:
- The result contains the reported event, e.g.
[[2590666, "dyncat", "obj", "method"]]
- The
setEventRecordingEnabled
call generated the following warning:
Unknown category for SetEventRecordingEnabled: dyncat
If I replace registerEvents
with registerBuiltinEvents
, then the expected result happens.
Reporter | ||
Comment 1•5 years ago
|
||
Is this behavior intentional? If so, can it be documented?
I found this issue while looking into bug 1561012. It is somewhat surprising that the behavior of setEventRecordingEnabled
depends on how the event was registered.
Comment 2•5 years ago
|
||
(In reply to Rob Wu [:robwu] from comment #1)
Is this behavior intentional? If so, can it be documented?
I found this issue while looking into bug 1561012. It is somewhat surprising that the behavior of
setEventRecordingEnabled
depends on how the event was registered.
Redirecting this, as Jan-Erik is out.
Assignee | ||
Comment 3•5 years ago
|
||
A quick note: Please do not use registerBuiltinEvents
. It isn't supposed to be used outside of Build Faster (artifact builds) workflows.
The behaviour might be intentional, or it might be an oversight. The code in question predates the concept of dynamically-registered events. I'll ni?gfritzsche in case he knows which it might be, but I'm of the opinion that we could change this if it is needed.
Is disabling the category of a dynamic event a useful feature to you, Rob?
Reporter | ||
Comment 4•5 years ago
|
||
(In reply to Chris H-C :chutten from comment #3)
A quick note: Please do not use
registerBuiltinEvents
. It isn't supposed to be used outside of Build Faster (artifact builds) workflows.
I'm aware of this. That's why I suggested to register in Events.yaml instead (in bug 1561012 ).
Is disabling the category of a dynamic event a useful feature to you, Rob?
The main reason for reporting this bug is to clarify the intended usage/contract of the API. I have no strong preferences here, though I would welcome consistency and reasonable API behavior. If it does not take too much efforts, then I would suggest to support disabling a dynamic event. If it takes too much efforts, then clarifying the documentation would be fine too.
The uses of .registerEvents(
are:
- fxmonitor - can/should be in Events.yaml - bug 1561012
telemetry
extension API for internal use.telemetry.registerEvents
is only used by the (unmaintained) Firefox Price Tracker. The in-tree Screenshots add-on uses the "telemetry" API, but only uses scalars that are registered in Scalars.yaml.- Documentation and unit tests for:
- the internal telemetry API.
- "telemetry" extension API (which has no significant use at the moment).
- hybrid content telemetry (whose future is unclear - bug 1520491).
Out of the few uses of dynamic telemetry events above, only the in-tree fxmonitor example uses setEventRecordingEnabled
with a preference-dependent false
value (telemetry on by default).
Comment 5•5 years ago
|
||
I don't remember if specific choice was intentional, but i currently don't see a use-case for disabling dynamically registered events. From my perspective, just documenting the intended usage & limitations should be best - unless there are specific use-cases here we're not aware of.
My expectation would be that dynamic events are recorded exclusively from addons:
- They register them when they start. After that they should just work, without further action.
- When the addon stops or is uninstalled, it wouldn't record the events anymore, so disabling them seems unnecessary.
If there are any specific needs, having a matching unregisterEvents()
might be a better fit?
Assignee | ||
Comment 6•5 years ago
|
||
It seems as though, for now, the first most impactful step is to clarify the documentation about the current behaviour. Thank you for bringing this to our attention, Rob! I'll be leaving this unprioritized to get picked up during triage.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 7•5 years ago
|
||
Pushed by chutten@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9ea5e6e45f29 Clarify Event Telemetry docs about dynamic event categories r=janerik
Comment 9•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Updated•2 years ago
|
Description
•