1463694 - Changes to Events.yaml not available to browser mochitests when using artifact builds

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

1. In Events.yaml add an extra key, "shortcut", to devtools.main::open so it looks like this:

  ```
  devtools.main:
    open:
      objects: ["tools"]
      bug_numbers: [1416024, 1456984]
      notification_emails: ["[e-mail]", "[e-mail]"]
      record_in_processes: ["main"]
      description: User opens devtools toolbox.
      release_channel_collection: opt-out
      expiry_version: never
      extra_keys:
        entrypoint: How was the toolbox opened? CommandLine, ContextMenu, DeveloperToolbar, HamburgerMenu, KeyShortcut, SessionRestore or SystemMenu
        first_panel: The name of the first panel opened.
        host: "Toolbox host (positioning): bottom, side, window or other."
        splitconsole: Indicates whether the split console was open.
        width: Toolbox width rounded up to the nearest 50px.
        shortcut: The key combination pressed. Used only in the case that entrypoint === KeyShortcut.
```

2. rm -rf objdir.noindex # or your objdir
3. ./mach build # artifact build
4. Open the browser console.
5. Open a privileged scratchpad and paste this in:
  Services.telemetry.recordEvent("devtools.main", "open", "tools", null, {
    "entrypoint": "KeyShortcut",
    "first_panel": "Inspector",
    "host": "bottom",
    "splitconsole": "false",
    "width": "777",
    "shortcut": "shift+f9"
  })
6. The browser console will show "Invalid extra key for event ["devtools.main", "open", "tools"]."

So the shortcut property is not taking.

Comment 1

[Jan-Erik Rediger [:janerik]]

Right now we explicitely don't support changing the definitions of existing code [1] (we use the same code paths for dynamic and dynamic-builtin events), but don't document that outside of the code.

We could change this a bit and allow re-defining existing probes, to make above case easier (from what I see in the code it might be possible rather easily).

What do you think, :chutten?

[1]: https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/toolkit/components/telemetry/TelemetryEvent.cpp#559

Comment 2

[Chris H-C :chutten]

I concur that we could. As to whether we should...  well, there appear to be no problems with it on the client-side and there is at least one clear use case for it.

However, there may be pipeline pieces that assume event definitions are immutable (like Scalars and Histograms are). I don't think so. I'm pretty sure the pipeline at most cares about category+method+object (which UniqueEventName keeps us honest about in the code you linked) but Sunah would know better.

Comment 3

[Jan-Erik Rediger [:janerik]]

The pipeline would be unaffected. This is only a matter for dynamic builtin events ("build faster"), so only affects dev environments and not any released build.

Comment 4

[bugzilla]

:janerik is correct, we don't use Events.yaml in the pipeline (yet)

Comment 5

[Jan-Erik Rediger [:janerik]]

Removing P1 for now, will get triaged tomorrow.
Current workaround is doing a full-build.

Comment 6

[Georg Fritzsche [:gfritzsche]]

There is at least one interesting use-case here that we should support:
- checkout, clobber
- add a new event
- do a full build
- iterate and change things on the event
- `mach build faster`

Jan-Erik, do you think this is easy to support?

The question of immutability of existing probes seems orthogonal and we should take that to a separate conversation.

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #6)
> There is at least one interesting use-case here that we should support:
> - checkout, clobber
> - add a new event
> - do a full build
> - iterate and change things on the event
> - `mach build faster`
> 
> Jan-Erik, do you think this is easy to support?
> 
> The question of immutability of existing probes seems orthogonal and we
> should take that to a separate conversation.

Please link to that conversation here as, having created a bunch of probes and having more planned I may have some useful input.

Comment 8

[Jan-Erik Rediger [:janerik]]

Yes, we should be able to update the definitions when a dynamic-builtin is registered.

Comment 9

[Georg Fritzsche [:gfritzsche]]

(In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from comment #7)
> > The question of immutability of existing probes seems orthogonal and we
> > should take that to a separate conversation.
> 
> Please link to that conversation here as, having created a bunch of probes
> and having more planned I may have some useful input.

We can take this to bug 1464806.

Comment 10

[Georg Fritzsche [:gfritzsche]]

(In reply to Jan-Erik Rediger [:janerik] from comment #8)
> Yes, we should be able to update the definitions when a dynamic-builtin is
> registered.

Is this something you can fit into this iteration?

Comment 11

[Jan-Erik Rediger [:janerik]]

Unless I'm running into unforeseen problems this should be an additional check and assignment. Getting on it tomorrow.

Comment 12

[Jan-Erik Rediger [:janerik]]

Attachment: Bug 1463694 - Allow dynamic builtin events to overwrite existing events on registration.

This means that during a normal "mach build faster" build all the events
will be overwritten, as the generate EventArtifactsDefinitions.json
contains all events from the Events.yaml it was generated from.

Real dynamic events follow similar code paths internally.
We also ensure they trigger the right code path and don't overwrite.

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

Comment 13

[Chris H-C :chutten]

Comment on attachment 8981868 [details]
Bug 1463694 - Allow dynamic builtin events to overwrite existing events on registration.

https://reviewboard.mozilla.org/r/247906/#review254032

Only some nits

::: commit-message-5866d:4
(Diff revision 1)
> +Bug 1463694 - Allow dynamic builtin events to overwrite existing events on registration. r?chutten
> +
> +This means that during a normal "mach build faster" build all the events
> +will be overwritten, as the generate EventArtifactsDefinitions.json

*generated

::: toolkit/components/telemetry/TelemetryEvent.cpp:551
(Diff revision 1)
>    for (uint32_t i = 0, len = eventInfos.Length(); i < len; ++i) {
>      const nsCString& eventName = UniqueEventName(eventInfos[i]);
>  
> -    // Re-registering events can happen when add-ons update, so we don't print warnings.
> -    // We don't support changing their definition, but the expiry might have changed.
> +    // Re-registering events can happen for two reasons:
> +    //
> +    // * When add-ons update; we don't print warnings.

That semicolon should probably be a period, and the comment about "we don't print warnings" should be up with the "for two reasons" phrase (since it applies to both reasons)

::: toolkit/components/telemetry/tests/unit/test_TelemetryEvents_buildFaster.js:330
(Diff revision 1)
> +  // Now check that the snapshot contains the expected data.
> +  let snapshot =
> +    Telemetry.snapshotEvents(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
> +  Assert.ok(("parent" in snapshot), "Should have parent events in the snapshot.");
> +
> +  let expected = [

Some day we should factor this out into some sort of checkExpectedEvents(snapshot.parent, expected) utility, but today doesn't have to be that day.

Comment 14

[Jan-Erik Rediger [:janerik]]

Comment on attachment 8981868 [details]
Bug 1463694 - Allow dynamic builtin events to overwrite existing events on registration.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/247906/diff/1-2/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8f3c1319cca
Allow dynamic builtin events to overwrite existing events on registration. r=chutten

Comment 16

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f8f3c1319cca