1561017 - Cannot use telemetry.setEventRecordingEnabled to disable a dynamic telemetry category

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Rob Wu [:robwu]]

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.

Comment 1

[Rob Wu [:robwu]]

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

[Alessio Placitelli [:Dexter]]

(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.

Comment 3

[Chris H-C :chutten]

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?

Comment 4

[Rob Wu [:robwu]]

(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

[Georg Fritzsche [:gfritzsche]]

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?

Comment 6

[Chris H-C :chutten]

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.

Comment 7

[Chris H-C :chutten]

Attachment: Bug 1561017 - Clarify Event Telemetry docs about dynamic event categories r?janerik

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ea5e6e45f29
Clarify Event Telemetry docs about dynamic event categories r=janerik

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/9ea5e6e45f29