Closed Bug 1460400 Opened 2 years ago Closed 2 years ago

The categories of all Dynamic Builtin Events are enabled

Categories

(Toolkit :: Telemetry, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: chutten, Assigned: janerik)

References

Details

Attachments

(1 file)

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.
Assignee: nobody → chutten
Priority: -- → P1
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.
Popping this back out for triage. It might be enough now that we know what is happening and the shape of the problem.
Assignee: chutten → nobody
Priority: P1 → --
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.
janerik expressed a desire to look at this next week.

Until then I recommend you test all the "events are enabled" things first :)
Assignee: nobody → jrediger
Status: NEW → ASSIGNED
Priority: -- → P1
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?
Attachment #8975445 - Flags: review?(chutten) → review+
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.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/843b90da1fad
Don't enable dynamic builtin events on registration. r=chutten
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/843b90da1fad
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
I know this was a while ago, but I just wanted to confirm that it works now as expected. Thank you!
See Also: → 1561012
Regressions: 1561017
You need to log in before you can comment on or make changes to this bug.