Closed
Bug 1438239
Opened 7 years ago
Closed 7 years ago
Using unknown telemetry event pings
Categories
(Firefox :: New Tab Page, enhancement, P2)
Firefox
New Tab Page
Tracking
()
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.
Reporter | ||
Comment 1•7 years ago
|
||
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)
Comment 2•7 years ago
|
||
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)
Reporter | ||
Comment 3•7 years ago
|
||
> 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.
Comment 4•7 years ago
|
||
Worth noting this is also the case with drag and drop:
Unknown event: ["activity_stream", "event", "DRAG"]
Unknown event: ["activity_stream", "event", "DROP"]
Updated•7 years ago
|
Iteration: --- → 61.1 - Mar 26
Priority: -- → P3
Updated•7 years ago
|
Iteration: 61.1 - Mar 26 → ---
Updated•7 years ago
|
Iteration: --- → 61.2 - Apr 9
Priority: P3 → P2
Updated•7 years ago
|
Comment 5•7 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 9•7 years ago
|
||
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)
Comment 10•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
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"
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8964965 [details]
Bug 1438239 - Update Events.yaml for Activity Stream.
https://reviewboard.mozilla.org/r/233688/#review239746
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 14•7 years ago
|
||
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 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8964965 [details]
Bug 1438239 - Update Events.yaml for Activity Stream.
https://reviewboard.mozilla.org/r/233688/#review239774
Attachment #8964965 -
Flags: review+
Comment 16•7 years ago
|
||
Pushed by usarracini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7988c47de43d
Update Events.yaml for Activity Stream. r=emtwo,ursula
Comment 17•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Reporter | ||
Updated•6 years ago
|
Updated•5 years ago
|
Component: Activity Streams: Newtab → New Tab Page
You need to log in
before you can comment on or make changes to this bug.
Description
•