Closed Bug 1438239 Opened 2 years ago Closed 2 years ago

Using unknown telemetry event pings

Categories

(Firefox :: New Tab Page, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 61
Iteration:
61.2 - Apr 9
Tracking Status
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: andreio, Assigned: nanj)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Unknown event: ["activity_stream", "event", "TOP_SITES_ADD_FORM_OPEN"]
Unknown event: ["activity_stream", "event", "TOP_SITES_EDIT_CLOSE"]

When opening/closing the TopSiteEdit modal.
More info about this: bug 1429489 made it so that we use the Firefox telemetry pipeline but not all events are registered in the Events.yaml [0]. And the error results in the event being dropped entirely.

One recommendation I got for making event handling more flexible was using the value field [1] (that doesn't require validation) to send the event names. This way the "objects" field would instead consist of various AS components that don't change that often.
Example:
{
  category: "activity_stream",
  method: "event",
  object: "modal_form",
  value: "TOP_SITES_ADD_FORM_OPEN"
}

[0] https://searchfox.org/mozilla-central/rev/4234703a532006c5ef9ce09b4c487d88124526a0/toolkit/components/telemetry/Events.yaml#4-19
[1] https://searchfox.org/mozilla-central/rev/4234703a532006c5ef9ce09b4c487d88124526a0/toolkit/components/telemetry/TelemetryEvent.cpp#760-761
Flags: needinfo?(msamuel)
Are the components we would use as objects (e.g. "modal_form") already defined somewhere within a-s? They sound a bit similar to "source" which is now what we set "value" to.

And if we use make object -> source and value -> event (essentially flipping them from the way they are now) I'm not sure we won't come across the same issue.

Alternatively, "object" could be empty and value -> event. But then "source" would go into the "extras" mapped json portion of the ping and be a little harder to query...

I think to resolve this bug we can simply update Events.yaml for now and we need a follow-up to reconsider the full format of the ping.
Flags: needinfo?(msamuel)
>  I'm not sure we won't come across the same issue.

The idea was that, for example, we will always have a modal component, we wouldn't need to update Events.yaml if the modal component gained new functionality because we can just set the "value" to the new type of event.
But I'm not very familiar with the issue.
Worth noting this is also the case with drag and drop:
Unknown event: ["activity_stream", "event", "DRAG"]
Unknown event: ["activity_stream", "event", "DROP"]
Iteration: --- → 61.1 - Mar 26
Priority: -- → P3
Iteration: 61.1 - Mar 26 → ---
Iteration: --- → 61.2 - Apr 9
Priority: P3 → P2
Blocks: 1445083
Severity: normal → enhancement
Assignee: nobody → najiang
This also the case when you archive or delete highlights from Pocket, accept the Pocket notice, start the history migration or cancel it:

Unknown event: ["activity_stream", "event", "ARCHIVE_FROM_POCKET"]
Unknown event: ["activity_stream", "event", "DELETE_FROM_POCKET"]
Unknown event: ["activity_stream", "event", "SECTION_DISCLAIMER_ACKNOWLEDGED"]
Unknown event: ["activity_stream", "event", "MIGRATION_START"]
Unknown event: ["activity_stream", "event", "MIGRATION_CANCEL"]
Comment on attachment 8964965 [details]
Bug 1438239 - Update Events.yaml for Activity Stream.

https://reviewboard.mozilla.org/r/233688/#review239312
Attachment #8964965 - Flags: review?(msamuel) → review+
Hey Francois,

Could you take a look at this patch please? It's simply a 1-to-1 mapping between Ping-Centre and Universal Event Telemetry, which were already reviewed before without any additional fields added.

A similar previous review for your reference: https://bugzilla.mozilla.org/show_bug.cgi?id=1429497#c9

Thanks,
Flags: needinfo?(francois)
Based on your explanations and Rebecca's previous comment on bug 1429497, I agree that this doesn't need a data review. The data that's being collected has already gone through a separate data review.
Flags: needinfo?(francois)
Turns out UT event employs a hard size limit (20) on the event value, and following events need to be trimmed accordingly. Will file a separate bug to fix this in AS.

"SECTION_DISCLAIMER_ACKNOWLEDGED",
"SECTION_MENU_ADD_TOPSITE",
"SECTION_MENU_COLLAPSE",
"SECTION_MENU_MOVE_DOWN",
"SECTION_MENU_PRIVACY_NOTICE"
Blocks: 1451749
Comment on attachment 8964965 [details]
Bug 1438239 - Update Events.yaml for Activity Stream.

https://reviewboard.mozilla.org/r/233688/#review239746
Keywords: checkin-needed
This doesn't have meet the review requirements in MozReview for Autoland to push it.
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/autoland.html#landing-commits
Keywords: checkin-needed
Comment on attachment 8964965 [details]
Bug 1438239 - Update Events.yaml for Activity Stream.

https://reviewboard.mozilla.org/r/233688/#review239774
Attachment #8964965 - Flags: review+
Pushed by usarracini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7988c47de43d
Update Events.yaml for Activity Stream. r=emtwo,ursula
https://hg.mozilla.org/mozilla-central/rev/7988c47de43d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
See Also: → 1471523
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.