1429497 - Update Events.yaml with Activity Stream events

Status: RESOLVED FIXED

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

Description

[Marina Samuel [:emtwo]]

This bug is a prerequisite for bug 1429489 to be able to send the telemetry.

The schema we'll be using needs to be added here: https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/Events.yaml

Comment 1

[Marina Samuel [:emtwo]]

Attachment: Bug 1429497 - Update Events.yaml with Activity Stream click and session events.

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

Comment 2

[Nan Jiang [:nanj]]

Comment on attachment 8941972 [details]
Bug 1429497 - Update Events.yaml with Activity Stream click and session events.

https://reviewboard.mozilla.org/r/212162/#review218296

::: toolkit/components/telemetry/Events.yaml:2
(Diff revision 1)
> +activity_stream:
> +  CLICK:

Does it make sense to generalize this method as `event` so that it also captures other events such as `DELETE`, `EDIT` etc. besides `CLICK`?

If event telemetry prefers to split the events into individual `methods`, then just ignore the previous comment.

::: toolkit/components/telemetry/Events.yaml:18
(Diff revision 1)
> +      addon_version: The Activity Stream addon version.
> +      session_id: The ID of the Activity Stream session in which the event occurred
> +      page: about:home or about_newtab - the page where the event occurred
> +      user_prefs: An integer representaing a user's A-S settings.
> +  SESSION:
> +    objects: ["END"]

nit: shall we exchange the `method` and `object` here?

```
activity_stream:
  END:
     objects: ["session"]
```

Comment 3

[Marina Samuel [:emtwo]]

(In reply to Nan Jiang [:nanj] from comment #2)
> ::: toolkit/components/telemetry/Events.yaml:2
> (Diff revision 1)
> > +activity_stream:
> > +  CLICK:
> 
> Does it make sense to generalize this method as `event` so that it also
> captures other events such as `DELETE`, `EDIT` etc. besides `CLICK`?
> 
> If event telemetry prefers to split the events into individual `methods`,
> then just ignore the previous comment.

Yes we can do this. Then instead of "action_position" being a value in the main payload, it would go in the "extras" field. Instead "object" would be "CLICK", "BLOCK", etc and "value" would be the "source" from the ping centre payload.

> ::: toolkit/components/telemetry/Events.yaml:18
> > +  SESSION:
> > +    objects: ["END"]
> 
> nit: shall we exchange the `method` and `object` here?
> 
> ```
> activity_stream:
>   END:
>      objects: ["session"]
> ```

Thanks! That's a good point. I suppose it's true that the "method" is the action occurring which is an "end" and it happens on the "session" object. I'll make this change.

Comment 4

[Marina Samuel [:emtwo]]

Comment on attachment 8941972 [details]
Bug 1429497 - Update Events.yaml with Activity Stream click and session events.

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

Comment 5

[Marina Samuel [:emtwo]]

Comment on attachment 8941972 [details]
Bug 1429497 - Update Events.yaml with Activity Stream click and session events.

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

Comment 6

[Nan Jiang [:nanj]]

Comment on attachment 8941972 [details]
Bug 1429497 - Update Events.yaml with Activity Stream click and session events.

https://reviewboard.mozilla.org/r/212162/#review218624

::: toolkit/components/telemetry/Events.yaml:23
(Diff revision 3)
> +      "UNPIN",
> +      "SAVE_TO_POCKET"]
> +    release_channel_collection: opt-out
> +    record_in_processes: ["main"]
> +    description: >
> +      This is recorded with every click on Activity Stream elements.

perhaps s/click/every user interaction/

::: toolkit/components/telemetry/Events.yaml:47
(Diff revision 3)
> +    notification_emails:
> +      - "[e-mail]"
> +      - "[e-mail]"
> +    expiry_version: never
> +    extra_keys:
> +      addon_version: The Activity Stream addon version.

Shall we add a "session_duration" here?

Comment 7

[Marina Samuel [:emtwo]]

Comment on attachment 8941972 [details]
Bug 1429497 - Update Events.yaml with Activity Stream click and session events.

https://reviewboard.mozilla.org/r/212162/#review218624

> Shall we add a "session_duration" here?

As discussed on irc, "session_duration" is sent as the ping "value" so we don't need to add it here.

Comment 8

[Marina Samuel [:emtwo]]

We would like to replicate existing Activity Stream data that is being sent to the Tiles pipeline so that it's also sent to the Events Telemetry pipeline. The motivation is to gradually shift off of the Tiles pipeline entirely.

This bug and bug 1429489 are the initial bits of work for this.

I'd like to request data review for these bugs please. Do I need to fill out a form or are we ok without that since the data is already being collected in Tiles?

Thanks!

Comment 9

[[e-mail]]

:emtwo since the data is being collected already in Tiles and onto Telemetry, I don't see a reason for additional data review (as long as you aren't adding any additional extra data collection).

Comment 10

[Marina Samuel [:emtwo]]

Comment on attachment 8941972 [details]
Bug 1429497 - Update Events.yaml with Activity Stream click and session events.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/212162/diff/3-4/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f2c4c0106576
Update Events.yaml with Activity Stream click and session events. r=nanj

Comment 12

[Narcis Beleuzu [:NarcisB]]

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