1460400 - The categories of all Dynamic Builtin Events are enabled

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Chris H-C :chutten]

The contents of EventArtifactDefinitions.json contains the entirety of Events.yaml, in a format to be passed to `registerEvents` in `TelemetryController` at startup.

Unfortunately, using registerEvents to register built-in events means that all built-in events are becoming enabled on these builds. This is contrary to how they're supposed to work where they are disabled by default until setEventRecordingEnabled is called for their particular category.

(( Actually-distributed builds don't have EventArtifactDefinitions.json, which means we're only doing this for developers building and running their own builds locally. ))

It seems as though the most straightforward fix for this might be to ensure that dynamic builtins are registered without enabling their categories.

Comment 1

[Chris H-C :chutten]

Alas it isn't as straightforward as I'd hoped as to be able to enable a category later it must be known to TelemetryEvents core through the gCategoryNameIDMap, which is immutable after init.

Comment 2

[Chris H-C :chutten]

Popping this back out for triage. It might be enough now that we know what is happening and the shape of the problem.

Comment 3

[Bianca Danforth [:bdanforth]]

I encountered this while working on a patch that enables/disables an event telemetry category behind a pref.

This bug, while not a blocker, does prevent QA/me from verifying that my patch alone controls enabling my event telemetry category with a local/developer build. The only known, confirmed workaround is to push a non-artifact try build and download it from Treeherder. This makes testing/checking related changes quite a bit slower (~45 minutes rather than ~45 seconds) in addition whatever costs there are for using our test infrastructure.

Comment 4

[Chris H-C :chutten]

janerik expressed a desire to look at this next week.

Until then I recommend you test all the "events are enabled" things first :)

Comment 5

[Jan-Erik Rediger [:janerik]]

Attachment: Bug 1460400 - Don't enable dynamic builtin events on registration.

This makes sure dynamic builtin events follow the same semantics as static builtin events.
On registration of the event the category is stored, but not enabled.
For fully-dynamic events, e.g. those registered by addons, the category
is enabled immediately (and can't be disabled).

This removes now-unused type definitions and switches from a map to a
simple set to store the category names.
The value stored in the map previously was not used at all.
In theory the map was effectively immutable after initialization, but
the check was only forced in debug anyway.
Now the set is mutable, but is only mutated in exactly 2 places.

Review commit: https://reviewboard.mozilla.org/r/243736/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243736/

Comment 6

[Chris H-C :chutten]

Comment on attachment 8975445 [details]
Bug 1460400 - Don't enable dynamic builtin events on registration.

https://reviewboard.mozilla.org/r/243736/#review249622

I have a question about behaviour which isn't really tied to this patch, but I think maybe we should put a Note in the docs or something about it if true.

::: toolkit/components/telemetry/TelemetryEvent.cpp:581
(Diff revision 1)
> +  }
> +
> +  if (!aBuiltin) {
> +    // Now after successful registration enable recording for this category
> +    // (if not a dynamic builtin).
> -  gEnabledCategories.PutEntry(category);
> +    gEnabledCategories.PutEntry(category);

So does this mean if I have a categoriy "navigation" with events on it statically registered, and I dynamically register a "navigation" category I will enable that category?

Comment 7

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8975445 [details]
Bug 1460400 - Don't enable dynamic builtin events on registration.

https://reviewboard.mozilla.org/r/243736/#review249622

> So does this mean if I have a categoriy "navigation" with events on it statically registered, and I dynamically register a "navigation" category I will enable that category?

Yes, that enables the full category. That is the same behaviour as before we had dynamic builtins.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/843b90da1fad
Don't enable dynamic builtin events on registration. r=chutten

Comment 9

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/843b90da1fad

Comment 10

[Bianca Danforth [:bdanforth]]

I know this was a while ago, but I just wanted to confirm that it works now as expected. Thank you!