Make it easier to test events through TelemetryTestUtils

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P1
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: gfritzsche, Assigned: chutten)

Tracking

unspecified
mozilla67
Points:
3

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(5 attachments, 3 obsolete attachments)

Reporter

Description

3 months ago

Improve Telemetry events testing API

Reporter

Comment 2

3 months ago
This is just a patch i tested locally that the first patch is based on.
Reporter

Comment 3

3 months ago

I was looking through our events usage in Firefox, including how we test them.
I found that it takes too much knowledge and two extra steps on the testing engineers side to set things up.
So this is sketching out an idea to make the event testing in TelemetryTestUtils easier to use.

This might be nice to support the upcoming Firefox event instrumentations (given us a concrete motivation and use-case).

Reporter

Updated

3 months ago
Summary: Bug xxx - Make it easier to test events through TelemetryTestUtils → Make it easier to test events through TelemetryTestUtils
Reporter

Updated

3 months ago
Priority: -- → P1
Assignee

Comment 4

3 months ago

Step 1 is to research devtools', Usage Telemetry's, telemetry xpcshell tests' testing of Events for ideas for API shapes.

Assignee: nobody → chutten
Status: NEW → ASSIGNED
Points: --- → 3
Assignee

Comment 5

3 months ago

Research:

devtools:

  • documents how to test Event Telemetry. (Which means if we make this form of test easier we may wish to update the docs)
  • Tests are most often of the form "filter to only records that match these category,method,object tuples, and ensure they have the exact composition and order in [EXPECTED_ARRAY]"
  • They have their own helper for checking a solitary event, allowing a pass if the session_id matches a form.
  • Not at all interested in non-parent events
  • Likes transforming the raw event array to a struct with named members. Interesting, and much more readable.

Cert Error and Tracking UI:

  • I see some exact string matching, mostly interested in solitary events filtered by exact category, object, and method,

Browser Usage Telemetry:

  • Uses TelemetryTestUtils.assertEvents directly
  • Also checks once or twice that no events match. Interesting case.

Uptake Telemetry:

  • Also transforms the raw arrays to structs like devtools does.
  • Has its own helpers which makes a lot of sense because it's still only testing the histogram version and not the event version. I expect it'll be converted soonish.

Extensions:

  • Also has its own helpers of the form "filter to known category,method,object and check exact equality" (in this case using Assert.deepEqual.

Normandy:

And I have to go catch my bus. Tomorrow: Telemetry's own Events tests.

Assignee

Comment 6

3 months ago

Telemetry:

  • Performs validation of the returned event structure, like checking the data types (we can ignore for a utils method)
  • Validates that Event Summary counts the right number of events (we can ignore for a utils method, though double-checking against Event Summaries might not be a bad idea if, for instance, we can't find an event in a snapshot)
  • Tests registration (can ignore)
  • Checks TELEMETRY_EVENT_RECORDING_ERROR... might be interesting to use this similarly to Event Summary counts.
  • When it checks events in bulk it uses raw arrays and doesn't filter
  • Tests dynamically-registered events

mozapps/extensions:

  • Like devtools and uptake filters on category,method.
  • Unlike them, it matches the raw arrays instead of structs with named fields
Assignee

Comment 7

3 months ago

In summary it seems as though there is some broad demand for a Telemetry Events test utility that filters the snapshot on category,method,object (or some subset of them) and either matches the result against a list of known events (or parts of the events) or asserts that there's the right number of them (the right number might be 0).

The list of known events could be a raw array but seems to be nicer if it is a struct with named fields. The field values might be strings to check against, RegExps to exec, or functions to call with the value.

Only we (Telemetry) ever want to not clear events on snapshot. Everyone else either does it explicitly with the final true parameter or implicitly by only snapshotting once. So we can take care of that for callers, so long as we're clear about it.

Only we (Telemetry) seem to care about dataset at all, so this utility can do away with the whole optin/optout/base/extended mess and always snapshot every event for filter and comparison.

There do exist callers that want to check non-parent-process events, so we should probably oblige.

--

I think we can work with this.

It's tempting to do it all in one, where the list of expected events contains the category,method,object we'll first filter on and so on... but if so many people in so many places think about testing events as "filter first, then match" then the API should match.

Comment hidden (obsolete)
Assignee

Updated

3 months ago
Attachment #9043253 - Attachment is obsolete: true
Assignee

Updated

3 months ago
Attachment #9043254 - Attachment is obsolete: true
Assignee

Comment 10

3 months ago

This adds a general-purpose testing API for testing Telemetry Events, and a
handy extra API (assertNumberOfEventsAndClear) for if you just want to count.

Assignee

Comment 11

3 months ago

Also, it turns out we no longer debug assert when recording too many events, so
we can turn on test_event_summary_limit in debug.

Depends on D20863

Attachment #9046021 - Attachment description: Bug 1527299 - Add TelemetryTestUtils.assertEventsAndClear r?janerik → Bug 1527299 - Add TelemetryTestUtils.assertEvents r?janerik
Attachment #9046022 - Attachment description: Bug 1527299 - Convert Telemetry Event tests to assertEventsAndClear r?janerik → Bug 1527299 - Convert Telemetry Event tests to assertEvents r?janerik

Comment 15

3 months ago
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/574433d7795e
Add TelemetryTestUtils.assertEvents r=janerik
https://hg.mozilla.org/integration/autoland/rev/034dce9d9393
Convert Telemetry Event tests to assertEvents r=janerik
https://hg.mozilla.org/integration/autoland/rev/30cf58a30352
Add a short explanation of Event Telemetry Testing to the docs. r=janerik
https://hg.mozilla.org/integration/autoland/rev/0b75d7939fe2
Update devtools' Event Telemetry doc and example r=miker
https://hg.mozilla.org/integration/autoland/rev/5dc14c7be626
Update callers of assertEvents to new form r=Standard8
You need to log in before you can comment on or make changes to this bug.