Closed Bug 1456415 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: Telemetry, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: miker, Assigned: janerik)

References

Details

Attachments

(3 files, 2 obsolete files)

Now that bug 1448945 is fixed, changes in Events.yaml are applied during an artifact build.

Unfortunately, these changes are still not visible to browser mochitests without performing a full build.
Assignee: nobody → jrediger
Priority: -- → P1
I tried to reproduce that, but was not successful.

Here's what I did:

* First a full build `./mach build`
* Then I patched the code, including adding new probes to `Events.yaml` and `Scalars.yaml`
* Triggered a faster build: `./mach build faster`
* Verified that the new probes are added in `obj/dist/bin/EventArtifactDefinitions.json` and `obj/dist/bin/ScalarArtifactDefinitions.json`
* Run the new test: `./mach test toolkit/components/telemetry/tests/browser/browser_DynamicBuiltinProbes.js`

The test pass, making me believe the dynamic builtin probes are loaded correctly.

If you use a different workflow to reproduce this, please let me know.
Flags: needinfo?(mratcliffe)
STR:

1. Add the following to Events.yaml:

   ```
   artifact.issue:
     testme:
       objects: ["test"]
       bug_numbers: [1456415]
       notification_emails: ["dev-developer-tools@lists.mozilla.org", hkirschner@mozilla.com"]
       record_in_processes: ["main"]
       description: This will not be picked up by an artifact build.
       release_channel_collection: opt-out
       expiry_version: never
       extra_keys:
         message: a message
   ```

2. Perform an artifact build.
3. Open the DevTools toolbox settings and ensure "Enable browser chrome and add-on debugging toolboxes."
4. Close DevTools.
3. Open Tools -> Web Developer -> Scratchpad.
2. From the Environment menu choose Browser context.
5. Enter the following code:

  ```
  Services.telemetry.recordEvent("artifact.issue", "testme", "test", null, {
    message: "hope this works"
  });
  Services.telemetry.snapshotEvents(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTOUT, false);
  ```
6. Click Inspect.
7. You can see that the event object is empty... it doesn't matter how many times you click Inspect, it is still empty.

Seems like something strange is happening though because about:telemetry does contain the events.
Flags: needinfo?(mratcliffe)
It works with `DATASET_RELEASE_CHANNEL_OPTIN`.
:chutten, can you clarify if that is expected behavior?
Flags: needinfo?(chutten)
Unfortunately DATASET_RELEASE_CHANNEL_OPTIN is the "give me all the things" option. It is confusing, and counter-intuitive, and we hope to make this better now that we've settled on on phrasing like CanRecordReleaseData, but we haven't gotten there yet.

That being said, if it is release_channel_collection: opt-out, it's in both of the datasets so it doesn't matter.

What -does- matter is that event recording is disabled by default, so for in-tree events you need to specifically enable the event category with a call to setEventRecordingEnabled: https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/events.html#seteventrecordingenabled

My guess is that this event is properly registered and the event is showing up as a keyed scalar thanks to Event Summary, but it isn't actually recorded.

Is this consistent with the evidence?
Flags: needinfo?(chutten)
There is still no data if I use:

```
Services.telemetry.setEventRecordingEnabled("artifact.issue", true);

Services.telemetry.recordEvent("artifact.issue", "testme", "test", null, {
  message: "hope this works"
});
Services.telemetry.snapshotEvents(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTOUT, false);
```

So the problem isn't that event recording is disabled by default.
The behavior is indeed different from a full build. I look into this.
MozReview-Commit-ID: CkreMOAFCP7
Attachment #8972842 - Flags: review?(chutten)
Comment on attachment 8972842 [details] [diff] [review]
Set correct record_on_release flag in generated JSON files. r?chutten

Review of attachment 8972842 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, so when we generated the dynamic builtin JSON we had 'record_on_release' being always false? Looks like a solid fix.

Can we get a test on this?
Attachment #8972842 - Flags: review?(chutten) → review+
MozReview-Commit-ID: IM9v7QS4Hiw
Attachment #8972903 - Flags: review?(chutten)
Comment on attachment 8972903 [details] [diff] [review]
Test generated artifact definitions json. r?chutten

Review of attachment 8972903 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. And this opens up the door to all sorts of new things we can test.
Attachment #8972903 - Flags: review?(chutten) → review+
Keywords: checkin-needed
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bc9caa50c3b
Set correct record_on_release flag in generated JSON files. r=chutten
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b8dadbdc7d0
Test generated artifact definitions json. r=chutten
Keywords: checkin-needed
MozReview-Commit-ID: IM9v7QS4Hiw
Attachment #8973197 - Flags: review?(chutten)
Attachment #8972903 - Attachment is obsolete: true
Flags: needinfo?(jrediger)
Comment on attachment 8973197 [details] [diff] [review]
Test generated artifact definitions json. r?chutten

Review of attachment 8973197 [details] [diff] [review]:
-----------------------------------------------------------------

Seems fine by me
Attachment #8973197 - Flags: review?(chutten) → review+
Keywords: checkin-needed
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1605fca9350
Set correct record_on_release flag in generated JSON files. r=chutten
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5329a24af4f
Test generated artifact definitions json. r=chutten
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c1605fca9350
https://hg.mozilla.org/mozilla-central/rev/c5329a24af4f
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
A first rough patch to mock the gzip compression as :chutten mentioned.
With a (hopefully) quickfix in bug 1331445, this patch would reenable the big ping test on Android by mocking the gzip compression using a not-so-random long string.
I am currently not able to push to try, so this will be hold off until I can do so anyway.
Attachment #8975822 - Flags: review?(chutten)
Attachment #8975822 - Attachment is obsolete: true
Attachment #8975822 - Attachment is patch: false
Attachment #8975822 - Flags: review?(chutten)
You need to log in before you can comment on or make changes to this bug.