Closed Bug 1389424 Opened 7 years ago Closed 7 years ago

Integrate ping-centre in onboarding addon to send ping events

Categories

(Firefox :: Tours, defect, P3)

Product:
Firefox
Component:
Tours
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: gasolin, Assigned: gasolin)

References

Details

(Whiteboard: [photon-onboarding])

Attachments

(2 files, 1 obsolete file)

schema test file
64 bytes, text/plain
Details
Bug 1389424 - enable onboarding telemetry via ping-centre;
59 bytes, text/x-review-board-request
liuche
: review+
emtwo
: review+
Fischer
: review+
ritu
: approval-mozilla-beta+
Details 
As discussed in https://docs.google.com/document/d/1hc6RLoWglQ58Uq4n9ofjMLhwTSWQcs7q9K4AAmjk-ow/edit?ts=598c71bd#

We'd 

* write schema test refer to the schema and https://github.com/mozilla/activity-stream/blob/master/system-addon/test/schemas/pings.js
* Integrate ping-centre.min.js bundle in onboarding addon
* send overlay events and notification ping events to ping-centre server
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Whiteboard: [photon-onboarding][triage]
Attached file schema test file — 
Hi Nan, I've defined our schema with Joi-browser, please take a look.


I test the overlay session event with Insomnia (API test tool like postman), but I always get 400 BAD REQUEST... do you have any idea?


Here's the log

```
* Preparing request to https://onyx_tiles.stage.mozaws.net/v4/links/activity-stream
...
> POST /v4/links/activity-stream HTTP/1.1
> Host: onyx_tiles.stage.mozaws.net
> User-Agent: insomnia/5.7.4
> Accept: */*
> Accept-Encoding: deflate, gzip
> Content-Type: application/x-www-form-urlencoded
> Content-Length: 258
| event=overlay_session&addon_version=1.0&client_id=26288a14-5cc4-d14f-ae0a-bb01ef45be9c&locale=en-US&page=about%3Anewtab&session_id=005deed0-e3e4-4c02-a041-17405fd703f6&session_duration=4199&category=overlay_interactions&=&topic=firefox-onboarding-session&=&=
* upload completely sent off: 258 out of 258 bytes
< HTTP/1.1 400 BAD REQUEST
< Content-Type: application/json; charset=utf-8
< Date: Fri, 11 Aug 2017 09:26:42 GMT
< Content-Length: 0
< Connection: keep-alive
```
Flags: needinfo?(najiang)
Hey Fred,

The endpoint for PingCentre in stage should be

https://onyx_tiles.stage.mozaws.net/v3/links/ping-centre

Could you try this one out?

Also, I noticed Gareth had added a few comments in the schema document, have you guys figured that out yet? Let me know once the schema is ready to go, so that i can create the new tables on our end.

Thanks
Flags: needinfo?(najiang)
Thanks for feedback, using https://onyx_tiles.stage.mozaws.net/v3/links/ping-centre instead still get 400 BAD REQUEST

Here's the log

* Preparing request to https://onyx_tiles.stage.mozaws.net/v3/links/ping-centre
...
> POST /v3/links/ping-centre HTTP/1.1
> Host: onyx_tiles.stage.mozaws.net
...
> Content-Type: application/x-www-form-urlencoded
> Content-Length: 252
| event=overlay_session&addon_version=1.0&client_id=26288a14-5cc4-d14f-ae0a-bb01ef45be9c&locale=en-US&page=about%3Anewtab&session_id=005deed0-e3e4-4c02-a041-17405fd703f6&session_duration=4199&category=overlay_interactions&topic=firefox-onboarding-session
* upload completely sent off: 252 out of 252 bytes
< HTTP/1.1 400 BAD REQUEST
< Content-Type: application/json; charset=utf-8
< Date: Mon, 14 Aug 2017 00:54:09 GMT
< Content-Length: 0
< Connection: keep-alive
Flags: needinfo?(najiang)
Hmm, I just tried to ping it as following command, and it worked!

>> curl -i -X POST -d '{"event":"overlay_session","source":"NEW_TAB","client_id":"a7967bc5-38ea-dd48-bfe9-ee06ca94f7e1","addon_version":"1.1.8","locale":"en-US","page":"NEW_TAB","session_id":"{d8a1812e-9f38-184c-9eed-01b98d4363e5}","topic": "firefox-onboarding-session"}' https://onyx_tiles.stage.mozaws.net/v3/links/ping-centre

Can you double check your payload, it should be a JSON stringified string.
Flags: needinfo?(najiang)
thanks for providing the example, running curl command works (removed source & {} sign around session_id)

`curl -i -X POST -d '{"event":"overlay_session","client_id":"a7967bc5-38ea-dd48-bfe9-ee06ca94f7e1","addon_version":"1.0","locale":"en-US","page":"NEW_TAB","session_id":"d8a1812e-9f38-184c-9eed-01b98d4363e5","topic": "firefox-onboarding-session"}' https://onyx_tiles.stage.mozaws.net/v3/links/ping-centre`
Priority: -- → P3
Whiteboard: [photon-onboarding][triage] → [photon-onboarding]
Target Milestone: --- → Firefox 57
In prototype I can ping from addon via ping-centre now, need to fill some real data in later patch


The ping-centre lib needs to be modified to deal with 

the lib size issue (133k -> 11k)
https://github.com/mozilla/ping-centre/issues/74

and fix lack of JSON serialize for fetch body
https://github.com/mozilla/ping-centre/pull/75
And I found ping-centre source doesn't provide a way to track the session, any suggestion will be appreciated.
Since Onboarding telemetry is done in shield-study, I'll try to reuse rex's code if possible

https://github.com/mozilla/shield-onboarding/commit/04adfa7340142ef2c11f52a15baf152ca4c0dfe4
integrated with ping-centre module and probes are added, the next task is to fill in the real data based on event
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

The telemetry code could send out real data to the server now and I think its time to get some feedback. 
the uncovered part is to log sessions correctly and to pass the right tab type.
Attachment #8897772 - Flags: feedback?(rexboy)
Attachment #8897772 - Flags: feedback?(fliu)
Attachment #8897772 - Flags: feedback?(rexboy)
Attachment #8897772 - Flags: feedback?(fliu)
When we get `session_start` events, we need identify and track per tab sessions to provide meaningful telemetry data.

Activity Stream use `RemotePageManager` that could identify which tab is sending message via portID http://searchfox.org/mozilla-central/source/toolkit/modules/RemotePageManager.jsm#285

But framescript does not provide such thing. Mossop, do you know if we can get tab id or the index when we handle the callback from Services.mm.addMessageListener? (messageManager)
Flags: needinfo?(dtownsend)
(In reply to Fred Lin [:gasolin] from comment #23)
> When we get `session_start` events, we need identify and track per tab
> sessions to provide meaningful telemetry data.
> 
> Activity Stream use `RemotePageManager` that could identify which tab is
> sending message via portID
> http://searchfox.org/mozilla-central/source/toolkit/modules/
> RemotePageManager.jsm#285
> 
> But framescript does not provide such thing. Mossop, do you know if we can
> get tab id or the index when we handle the callback from
> Services.mm.addMessageListener? (messageManager)

The "target" the callback is passed is the browser element that the message came from.
Flags: needinfo?(dtownsend)
Now we can track session for tabs, I think its ready for review.

Hi Marina, please help take a look if we did telemetry with ping-centre right.

Hi François, please help the data review part.
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review184230

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:20
(Diff revision 13)
> +});
> +XPCOMUtils.defineLazyServiceGetter(this, "gUUIDGenerator",
> +  "@mozilla.org/uuid-generator;1", "nsIUUIDGenerator");
> +
> +const EVENT_MAP = {
> +  "notification-cta-click": {topic: "firefox-onboarding-event", category: "notification-interactions"},

Could we have some comments here saying about what are "firefox-onboarding-event, "internal", "firefox-onboarding-event"... so in the future others would know how to chose the right topic when adding new pings.

"notification-interactions" and "overlay-interactions" are inituitive for interactions with notification/overlay.

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:41
(Diff revision 13)
> +  eventProbe: null,
> +  state: {
> +    sessions: {},
> +  },
> +
> +  init(data) {

I was thinking should we rename `data` to `startupData` or change `data` to `extensionVersion`. In fact only looking at `init(data)` is fine. Then consider `process(data)` and `send(topic, event, data)` (for these 2 method, `data` are related), it may lead people to think `data` for `init` is related to `process` and `send` too. Or maybe have some comments could mitigate this if we wanted to keep `init(data)`.

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:41
(Diff revision 13)
> +  eventProbe: null,
> +  state: {
> +    sessions: {},
> +  },
> +
> +  init(data) {

I was thinking rename `data` to `startupData` or change `data` to `addonVersion`. In fact only looking at `init(data)` is ok. Then consider `process(data)` and `send(data)` which for these 2 methods `data` are related. It may lead people to think `data` for `init` is releated to `process` and `send` too. Maybe have some comments could mitigate this if we wanted to keep `data`.

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:44
(Diff revision 13)
> +  },
> +
> +  init(data) {
> +    this.sessionProbe = new PingCentre({topic: "firefox-onboarding-session"});
> +    this.eventProbe = new PingCentre({topic: "firefox-onboarding-event"});
> +    Object.assign(this.state, {

Usually we saw using `Object.assign` for some reason, is there any special reason for it? 
Could achieve this by 2 lines of the simple assign as well:
  ```
   this.state.addon_version = data.vaersion;
   this.state.locale =  Services.locale.getAppLocaleAsLangTag();
  ```
And nit technical speeaking, the object assign is slightly slower than the simple assign. Could we need the reason, thanks.

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:107
(Diff revision 13)
> +    let category = EVENT_MAP[event].category;
> +    let session_begin;
> +    switch (topic) {
> +      case "firefox-onboarding-session":
> +        switch (event) {
> +          case "onboarding-session":

The whole picture we are doing is like onboarding.js tells OnboardingTelemetry.jsm that a session begins/ends. Then OnboardingTelemetry.js sends the session ping back to tell the server about a seesion's begin, end, id, event type info and so on.
So probably "onboarding-session-end" here is clearer if we wanted to reuse the event arg passed from onboarding.js. If went for "onboarding-session", it might be likely people wouldn't know that it is for the moment the end of one session in onboaridng.js.

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:118
(Diff revision 13)
> +          case "notification-session":
> +            session_begin = notification_session_start;
> +            break;
> +        }
> +
> +        const tour_source = Services.prefs.getStringPref("browser.onboarding.state", ICON_STATE_DEFAULT);

We are getting the onboarding state but the variabl is about the tour source??

::: browser/extensions/onboarding/content/onboarding.js:343
(Diff revision 13)
>    });
>  }
> +
> +/**
> + * Template code for talking to `PingCentre`
> + * @param {Object} data the payload for the telemetry

We've documented `sendMessageToChrome(action, params)` expects `params` as Array. Please update `sendMessageToChrome` documentation or update `telemetry`.

::: browser/extensions/onboarding/content/onboarding.js:360
(Diff revision 13)
>      this.init(contentWindow);
>    }
>  
>    async init(contentWindow) {
>      this._window = contentWindow;
> +    // used for telemetry: a high number provides sparse ids to identify tabs

For "high", do you mean "large" or "highly precise"?

::: browser/extensions/onboarding/content/onboarding.js:361
(Diff revision 13)
>    }
>  
>    async init(contentWindow) {
>      this._window = contentWindow;
> +    // used for telemetry: a high number provides sparse ids to identify tabs
> +    // the session will renew after reload

Sorry but not sure what seesion key is used for from the comments. Could you elaberate more on this? Maybe some example would help. To me it seems like just an id tracking the current tab. Not everyone knows details about the Ping Center so that would good we comment what we are doing clear to reduce the possiblity that others breaks our code.

::: browser/extensions/onboarding/content/onboarding.js:403
(Diff revision 13)
>        // Don't show the overlay UI before we get to a better, responsive design.
>        this.destroy();
>        return;
>      }
>  
>      this._initUI();

Without reloading a tab, we may `_initUI` multiple times during resizing, is that still ok to send the "onboarding-session-start" ping multiple times?

::: browser/extensions/onboarding/content/onboarding.js:436
(Diff revision 13)
>  
>      this._initPrefObserver();
>      // Doing tour notification takes some effort. Let's do it on idle.
>      this._window.requestIdleCallback(() => this._initNotification());
> +    telemetry({
> +      event: "onboarding-session-start",

Is the timing of this "onboarding-session-start" ping important? If not, maybe we could do it inside the above `requestIdleCallback`.

::: browser/extensions/onboarding/content/onboarding.js:528
(Diff revision 13)
>          this.hideOverlay();
>          break;
>        case "onboarding-notification-close-btn":
>          this.hideNotification();
> +        telemetry({
> +          event: "notification-session",

"notification-session-start"is clear for notification start and "notification-cta-click" is clear for notification CTA button being clicked. What is this "notification-session" for? If it is for notification close button being clicked, probably notification-close-click" is clearer when viewing ping data on the ping center panel. But if this is for notification closed in general, probably need to move it into `hideNotification`

::: browser/extensions/onboarding/content/onboarding.js:537
(Diff revision 13)
>          this._removeTourFromNotificationQueue(this._notificationBar.dataset.targetTourId);
>          break;
>        case "onboarding-notification-action-btn":
>          let tourId = this._notificationBar.dataset.targetTourId;
>          this.showOverlay();
> +        telemetry({

Please move this ping after `gotoPage` for 2 reasons:
1. go to the wanted page as fast as possible
2. if something wrong happened during sending ping, as least `gotoPage` would succeed.

::: browser/extensions/onboarding/content/onboarding.js:539
(Diff revision 13)
>        case "onboarding-notification-action-btn":
>          let tourId = this._notificationBar.dataset.targetTourId;
>          this.showOverlay();
> +        telemetry({
> +          event: "notification-cta-click",
> +          tour_id: this._notificationBar.dataset.targetTourId,

Please reuse `tourId`

::: browser/extensions/onboarding/content/onboarding.js:683
(Diff revision 13)
>        this._notificationBar.remove();
>      }
>      this._tourItems = this._tourPages =
>      this._overlayIcon = this._overlay = this._notificationBar = null;
> +    telemetry({
> +      event: "onboarding-session",

We have "onboarding-session-start" so does this mean the end of onboarding session?

::: browser/extensions/onboarding/content/onboarding.js:702
(Diff revision 13)
>    }
>  
>    hideOverlay() {
>      this.toggleModal(this._overlay.classList.toggle("onboarding-opened"));
> +    telemetry({
> +      event: "overlay-session",

The similar question: is this for the end of overlay session? If for end, probably we need "overlay-session-end". For these similar questions, I was thinking, if I were unfmailiar with our code base, I might not associate "overlay-session" with the end of overlay session but the duration of the session. Then I would though ok we have "overlay-session-start" and the "overlay-session" duration.

::: browser/extensions/onboarding/content/onboarding.js:992
(Diff revision 13)
>      let tourMessage = this._notificationBar.querySelector("#onboarding-notification-tour-message");
>      tourMessage.textContent = notificationStrings.message;
>      this._notificationBar.classList.add("onboarding-opened");
>      this._window.document.body.appendChild(this._notificationBar);
>  
> +    telemetry({event: "notification-session-start", session_key: this._session_key});

nit: could we move this ping to the bottom after all the notification related works get done.
Attachment #8897772 - Flags: review?(fliu)
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review184230

> I was thinking should we rename `data` to `startupData` or change `data` to `extensionVersion`. In fact only looking at `init(data)` is fine. Then consider `process(data)` and `send(topic, event, data)` (for these 2 method, `data` are related), it may lead people to think `data` for `init` is related to `process` and `send` too. Or maybe have some comments could mitigate this if we wanted to keep `init(data)`.

renamed

> Usually we saw using `Object.assign` for some reason, is there any special reason for it? 
> Could achieve this by 2 lines of the simple assign as well:
>   ```
>    this.state.addon_version = data.vaersion;
>    this.state.locale =  Services.locale.getAppLocaleAsLangTag();
>   ```
> And nit technical speeaking, the object assign is slightly slower than the simple assign. Could we need the reason, thanks.

The direct assignment just not work for the initial version, but it works fine now, updated.

> The whole picture we are doing is like onboarding.js tells OnboardingTelemetry.jsm that a session begins/ends. Then OnboardingTelemetry.js sends the session ping back to tell the server about a seesion's begin, end, id, event type info and so on.
> So probably "onboarding-session-end" here is clearer if we wanted to reuse the event arg passed from onboarding.js. If went for "onboarding-session", it might be likely people wouldn't know that it is for the moment the end of one session in onboaridng.js.

We track session start and end but only send pings when session end, so the server will get single "onboarding/overlay/notification-session" event which includes both `session_start` and `session_end` timestamp. Will add the reason in comment of EVENT_MAP.

> We are getting the onboarding state but the variabl is about the tour source??

The field is used to identify how user open the overlay (through default logo or watermark), the number of open from notification can be retrieved via 'notification-cta-click event'.
will add comment above.

> We've documented `sendMessageToChrome(action, params)` expects `params` as Array. Please update `sendMessageToChrome` documentation or update `telemetry`.

add extra return params type in `sendMessageToChrome`

> "notification-session-start"is clear for notification start and "notification-cta-click" is clear for notification CTA button being clicked. What is this "notification-session" for? If it is for notification close button being clicked, probably notification-close-click" is clearer when viewing ping data on the ping center panel. But if this is for notification closed in general, probably need to move it into `hideNotification`

I have the same thought when I saw this entrypoint from shield study. Though putting in `hideNotification` will be triggered every time when `showOverlay` is called. So I decide to left it here for discussion.

> We have "onboarding-session-start" so does this mean the end of onboarding session?

same reason above

> The similar question: is this for the end of overlay session? If for end, probably we need "overlay-session-end". For these similar questions, I was thinking, if I were unfmailiar with our code base, I might not associate "overlay-session" with the end of overlay session but the duration of the session. Then I would though ok we have "overlay-session-start" and the "overlay-session" duration.

same reason above
Attachment #8897772 - Flags: review?(francois)
Attachment #8897772 - Flags: review?(fliu)
I don't have the necessary context/background on this. You might want to ask Rebecca Weiss instead.
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review184230

> We track session start and end but only send pings when session end, so the server will get single "onboarding/overlay/notification-session" event which includes both `session_start` and `session_end` timestamp. Will add the reason in comment of EVENT_MAP.

The thing confusing people here is that "onboarding-session" is different from "onboarding-session-end" (at least for me, my first thought is like not sure what is the right moment to notify "onboarding-session"). Maybe you had some discussion about this before. I will includ Tim to look at this as well since he has been involved too.

> I have the same thought when I saw this entrypoint from shield study. Though putting in `hideNotification` will be triggered every time when `showOverlay` is called. So I decide to left it here for discussion.

OK, will include Tim to discuss
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

Hi Tim,
Since you has been involved in the onboarding telemetry. That would great that you could look at the patch as well, thanks.
Attachment #8897772 - Flags: review?(timdream)
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review184230

> The thing confusing people here is that "onboarding-session" is different from "onboarding-session-end" (at least for me, my first thought is like not sure what is the right moment to notify "onboarding-session"). Maybe you had some discussion about this before. I will includ Tim to look at this as well since he has been involved too.

the reason we didn't send both session_begin/session_end sessoin event is due to AS team's(najiang) request. A single event save server space and make nosql query easier. 
The reason we send both `session_begin/session_end` column instead of one `session_duration`(the diff) like AS is due to Gareth's request to log sepcific time for analysis.

The updated PR also contains the `data_event.md`(just like https://github.com/mozilla/activity-stream/blob/master/docs/v2-system-addon/data_dictionary.md) which describes the meaning of each column. Hope that helps review.

> OK, will include Tim to discuss

I found we can still trigger notification-session in `hideNotification` via add extra `contains("class")` check. updated.
Hi chenxia, all telemetry data we send are in OnboardingTelemetry.jsm and documented in data_events.md, could you help the data review part?
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review185262

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:64
(Diff revision 16)
> +
> +  init(startupData) {
> +    this.sessionProbe = new PingCentre({topic: "firefox-onboarding-session"});
> +    this.eventProbe = new PingCentre({topic: "firefox-onboarding-event"});
> +    this.state.addon_version = startupData.version;
> +    this.state.locale = Services.locale.getAppLocaleAsLangTag();

According to https://github.com/mozilla/activity-stream/blob/master/docs/v2-system-addon/data_dictionary.md, onyx server seems able to log locale automatically.

If so we don't need send that redundant data to the  server. Checking with najiang.
See Also: → 1399792
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review185662

Some feedback that needs to be addressed first, and I do not think we should be collecting the IP addresses of users as part of the onboarding telemetry.

::: browser/extensions/onboarding/data_events.md:3
(Diff revision 19)
> +# Metrics we collect
> +
> +We adhere [Activity Stream's data collection policy](https://github.com/mozilla/activity-stream/blob/master/docs/v2-system-addon/data_events.md). Data about your specific browsing behaior or the sites you visit is **never transmitted to any Mozilla server**.  At any time, it is easy to **turn off** this data collection by [opting out of Firefox telemetry](https://support.mozilla.org/kb/share-telemetry-data-mozilla-help-improve-firefox).

typo: "We adhere to", "browsing behavior"

::: browser/extensions/onboarding/data_events.md:7
(Diff revision 19)
> +
> +We adhere [Activity Stream's data collection policy](https://github.com/mozilla/activity-stream/blob/master/docs/v2-system-addon/data_events.md). Data about your specific browsing behaior or the sites you visit is **never transmitted to any Mozilla server**.  At any time, it is easy to **turn off** this data collection by [opting out of Firefox telemetry](https://support.mozilla.org/kb/share-telemetry-data-mozilla-help-improve-firefox).
> +
> +## User event pings
> +
> +The Onboarding system add-on sends 2 types of pings to the backend (HTTPS POST) [Onyx server](https://github.com/mozilla/onyx) :

Can you describe briefly what Onyx is here? incl if it is a Mozilla-run service.

::: browser/extensions/onboarding/data_events.md:63
(Diff revision 19)
> +
> +| KEY | DESCRIPTION | &nbsp; |
> +|-----|-------------|:-----:|
> +| `addon_version` | [Required] The version of the Onboarding addon. | :one:
> +| `category` | [Required] Either ("overlay-interactions", "notification-interactions") to identify which kind of the interaction | :one:
> +| `client_id` | [Required] An identifier for this client. auto append by ping-centre module | :one:

Where is this client id used, and is it shared anywhere else? Where is it generated?

::: browser/extensions/onboarding/data_events.md:64
(Diff revision 19)
> +| KEY | DESCRIPTION | &nbsp; |
> +|-----|-------------|:-----:|
> +| `addon_version` | [Required] The version of the Onboarding addon. | :one:
> +| `category` | [Required] Either ("overlay-interactions", "notification-interactions") to identify which kind of the interaction | :one:
> +| `client_id` | [Required] An identifier for this client. auto append by ping-centre module | :one:
> +| `event` | [Required] The type of event. Any user defined string | :one:

Please clarify what "any user defined string" is here. This should ideally be limited to a set of strings that you actually list here or later in this doc, but right now, it's not clear what this string is and this description needs to be more clear.

::: browser/extensions/onboarding/data_events.md:66
(Diff revision 19)
> +| `addon_version` | [Required] The version of the Onboarding addon. | :one:
> +| `category` | [Required] Either ("overlay-interactions", "notification-interactions") to identify which kind of the interaction | :one:
> +| `client_id` | [Required] An identifier for this client. auto append by ping-centre module | :one:
> +| `event` | [Required] The type of event. Any user defined string | :one:
> +| `impression` | [Optional] An integer to record the current notification tour's impression. | :one:
> +| `ip` | [Auto populated by Onyx] The IP address of the client. | :two:

What is this used for, and how does it help improve onboarding? I really don't think we should collect this, unless you have a very specific need for this specific piece of information. I'm sure we can figure out another way to get the information that you need *without* collecting every client's IP address.

::: browser/extensions/onboarding/data_events.md:71
(Diff revision 19)
> +| `ip` | [Auto populated by Onyx] The IP address of the client. | :two:
> +| `locale` | The browser chrome's language (eg. en-US). | :two:
> +| `page` | [Required] One of ["about:newtab", "about:home"]| :one:
> +| `session_begin` | Time in (integer) milliseconds when onboarding/overlay/notification becoming visible. | :one:
> +| `session_end` | Time in (integer) milliseconds when onboarding/overlay/notification losing focus. | :one:
> +| `session_id` | [Required] The unique identifier for a specific session. | :one:

Please include where/how this id is generated, and if it is used anywhere else.
Attachment #8897772 - Flags: review?(liuche) → review-
Hi Ed, I'm doing data review for this telemetry and I had some questions about Onyx, which is being used for Firefox onboarding. I see you as a contributor for Onyx, so wondering if you can give me some context.

I see that we're collecting the user's IP as part of using Onyx, and I'm wondering if you can give me some context on if/why this is necessary? I'm a little confused by the usage of Onyx here but I assume it's to create the ping, but is there a way to not automatically collect the client IP address while using Onyx?
Flags: needinfo?(edilee)
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review185662

> Can you describe briefly what Onyx is here? incl if it is a Mozilla-run service.

Onyx is the ping centre server(telemetry for Activity Stream) which is a Mozilla-run service.

> Where is this client id used, and is it shared anywhere else? Where is it generated?

client_id is generated by ClientID module to provide an anonymous identifier for device, which is auto attached by ping-centre module

> Please clarify what "any user defined string" is here. This should ideally be limited to a set of strings that you actually list here or later in this doc, but right now, it's not clear what this string is and this description needs to be more clear.

allowed event strings are defined in EVENT_MAP in OnboardingTelemetry.jsm

> Please include where/how this id is generated, and if it is used anywhere else.

It's generated by gUUIDGenerator service to assign a random id to identify the specific user session when onboarding is inited/when overlay is opened/when notification is shown.
(In reply to Chenxia Liu [:liuche] from comment #43)
> I see that we're collecting the user's IP as part of using Onyx, and I'm
> wondering if you can give me some context on if/why this is necessary?
nanj can provide more details of Onyx and its related services, and my knowledge might be outdated from 3 years ago when it was used in production for the about:newtab Tiles feature. I believe the primary purposes of collecting the IP address is to detect malicious behavior and accurate aggregation (and transiently it was used for geo/region lookup). That raw data is more strictly stored and retained, and it's not directly queryable.
Flags: needinfo?(edilee) → needinfo?(najiang)
(In reply to Chenxia Liu [:liuche] from comment #43)

> I see that we're collecting the user's IP as part of using Onyx, and I'm
> wondering if you can give me some context on if/why this is necessary? I'm a
> little confused by the usage of Onyx here but I assume it's to create the
> ping, but is there a way to not automatically collect the client IP address
> while using Onyx?

Onyx is a Mozilla owned service to serve tiles for the current newtab in Firefox, it also receives all the telemetry from the newtab page as well as Activity Stream. It's operated and monitored by the Cloud Services team.

Onyx does use (with the permission) the IP address to infer user's geo information so that it could prepare the corresponding tiles for the country she lives in. However, Ping-centre will NOT store IP address in the database, where only authorized Mozilla employees can access the telemetry data, and all the raw logs are being strictly managed by the Ops team and will expire according to the Mozilla's data retention policy.
Flags: needinfo?(najiang)
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review185912

::: browser/extensions/onboarding/content/onboarding.js:697
(Diff revision 19)
>        this._notificationBar.remove();
>      }
>      this._tourItems = this._tourPages =
>      this._overlayIcon = this._overlay = this._notificationBar = null;
> +    telemetry({
> +      event: "onboarding-session",

just curious, why don't we name it onboarding-session-end?

::: browser/extensions/onboarding/content/onboarding.js:722
(Diff revision 19)
>        this._loadTours(this._tours);
>      }
>  
>      this.hideNotification();
>      this.toggleModal(this._overlay.classList.toggle("onboarding-opened"));
> +    telemetry({event: "overlay-session-begin", session_key: this._session_key});

let's break it into multiple line just as other places you called `telemtry()`.
Flags: qe-verify-
Marina, after integrate with ping-centre, we also want to support shield study telemetry when we trigger some A/B test. Activity Stream seems already did some shield study work, could you share the best practice with us?
Flags: needinfo?(msamuel)
Hi Fred,

When you run the shield study, will you be collecting telemetry through PingCentre then as well? If so, you can add a filter when sending a ping through PingCentre here: http://searchfox.org/mozilla-central/source/browser/modules/PingCentre.jsm#94

This will make it so that the ping only keeps track of a shield ID if it's relevant to your experiment. For activity stream, the shield experiment ID always contains a substring "activity-stream" so that is the filter we use. In your case it's likely "onboarding" but you can probably confirm with the shield team.

We can also use the UT data that shield collects automatically to look at how each variant performs with respect to over newtabs open, search counts, active ticks, etc. There is nothing extra you'll need to do for this, we would generate the dashboards on our end.
Flags: needinfo?(msamuel)
cool, I traced Activity Stream code and found AS allways add the filter
http://searchfox.org/mozilla-central/source/browser/extensions/activity-stream/lib/TelemetryFeed.jsm#366

So I did it too, Please kindly review if our implement on PingCentre is correct.
Flags: needinfo?(msamuel)
[Tracking Requested - why for this release]: this patch allows for detailed event telemetry for the Onboarding Tour.
status-firefox57: --- → affected
tracking-firefox57: --- → ?
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review184230

> I found we can still trigger notification-session in `hideNotification` via add extra `contains("class")` check. updated.

The key point is about what is supposed to be measured. If this ping was to measure the every notification session between start and end, then should be put here. If not, then should not be put here. Would you confirm this?
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review184818

::: browser/extensions/onboarding/content/onboarding.js:363
(Diff revision 15)
>    async init(contentWindow) {
>      this._window = contentWindow;
> +    // session_key is used for telemetry to track the current tab.
> +    // A large random number provides sparse ids.
> +    // The number will renew after reload the page.
> +    this._session_key = Math.floor(Math.random() * 100000);

This still may have collision since the purpose is really for id. Why not use timestamp?

::: browser/extensions/onboarding/content/onboarding.js:697
(Diff revision 22)
>        this._notificationBar.remove();
>      }
>      this._tourItems = this._tourPages =
>      this._overlayIcon = this._overlay = this._notificationBar = null;
> +    telemetry({
> +      event: "onboarding-session",

OK from the code/doc, here "onboarding-session" actullay is an internally-used event notification. Please use "onboarding-session-end" in onboarding.js and then send "onboarding-session" session ping in OnboardingTelemetry.jsm. Mixing these 2 is convenient but confusing and hacky. Let onboarding.js focus on passing internal notification to tell OnboardingTelemetry.jsm the moment of one event. OnboardingTelemetry.jsm should be in charge of mapping events to sessions and then send session pings back to the server. The same for the other pings.
Attachment #8897772 - Flags: review?(fliu)
Attachment #8915862 - Attachment is obsolete: true
Attachment #8915862 - Flags: review?(msamuel)
Attachment #8915862 - Flags: review?(liuche)
Attachment #8915862 - Flags: review?(fliu)
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review191980

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:32
(Diff revision 22)
> + *
> + * We send `session_begin` and `session_end` timestamps instead of `session_duration` diff because
> + * of analytics engineer's request.
> + */
> +const EVENT_WHITELIST = {
> +  // track when click the notification CTA button

What does CTA stand for?

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:57
(Diff revision 22)
> +};
> +const ICON_STATE_DEFAULT = "default";
> +const PROMPT_COUNT_PREF = "browser.onboarding.notification.prompt-count";
> +const ONBOARDING_ID = "onboarding";
> +
> +var OnboardingTelemetry = {

s/var/let

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:93
(Diff revision 22)
> +          this.state.sessions[session_key].page = page;
> +        }
> +        break;
> +      case "overlay-session-begin":
> +        if (session_key) {
> +          this.state.sessions[session_key].overlay_session_begin = Date.now();

Is it possible to get here and this.state.sessions[session_key] is still undefined? Does this event *always* occur after "onboarding-session-begin"?

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:98
(Diff revision 22)
> +          this.state.sessions[session_key].overlay_session_begin = Date.now();
> +        }
> +        break;
> +      case "notification-session-begin":
> +        if (session_key) {
> +          this.state.sessions[session_key].notification_session_begin = Date.now();

How come you don't keep "session_id" for notification and overlay? Do all sessions share the same session_id that's set by "onboarding-session-begin"?

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:135
(Diff revision 22)
> +          case "onboarding-session":
> +            session_begin = onboarding_session_begin;
> +            delete this.state.sessions[session_key];
> +            break;
> +          case "overlay-session":
> +            session_begin = overlay_session_begin;

How come you don't delete this.state.sessions[session_key] here?

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:149
(Diff revision 22)
> +        const tour_source = Services.prefs.getStringPref("browser.onboarding.state", ICON_STATE_DEFAULT);
> +        const session_end = Date.now();
> +        this.sessionProbe && this.sessionProbe.sendPing({
> +          addon_version,
> +          category,
> +          locale,

Same here, don't really need locale.

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:166
(Diff revision 22)
> +        this.eventProbe && this.eventProbe.sendPing({
> +          addon_version,
> +          category,
> +          event,
> +          impression,
> +          locale,

locale is actually set by PingCentre by default. So you shouldn't need this.

::: browser/extensions/onboarding/content/onboarding.js:365
(Diff revision 22)
>    async init(contentWindow) {
>      this._window = contentWindow;
> +    // session_key is used for telemetry to track the current tab.
> +    // A large random number provides sparse ids.
> +    // The number will renew after reload the page.
> +    this._session_key = Math.floor(Math.random() * 100000);

Another way you could do this is using the portID attribute that's available as "msg.target.portID" when chrom receives msg from content.

Activity Stream does this: https://github.com/mozilla/activity-stream/blob/e708b26ebfc03086bb8661a6bda107e77bfd4dee/system-addon/lib/ActivityStreamMessageChannel.jsm#L178
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review192370

In general this looks good and I'm going to manually test it as well. My main concern is around session start/end which I left a couple of comments on.
Attachment #8897772 - Flags: review?(msamuel)
(In reply to Tim Spurway [:tspurway] from comment #54)
> [Tracking Requested - why for this release]: this patch allows for detailed
> event telemetry for the Onboarding Tour.

Hello Tim: Unfortunately we are going to have to won't fix this for Firefox 57. The bar for 57 is dot release drivers, and this doesn't seem to fit that criteria.
status-firefox57: affected → wontfix
tracking-firefox57: ? → -
Hi Ritu,

Briefly spoke with Marcia on slack about this, but we wanted to defer to you on the uplift call for this bug. I was under the impression that we are doing uplifts until Beta 8 (10/11 deadline).

The push to have this is Fx 57 is to have telemetry attributes in 57 for the onboarding tour. This will give us data that would help the team iterate on the onboarding experience. Moreover, this feature of onboarding hasn't been fully tested with users in the wild. The data provided from telemetry could give us valuable feedback that would impact 57.
Flags: needinfo?(rkothari)
I believe changes to onboarding add-on fit in the onboarding work planned for 57 and therefore meet the 57 uplift criteria. I forgot to share this with Marcia, sorry my bad.
status-firefox57: wontfix → affected
tracking-firefox57: - → ---
Flags: needinfo?(rkothari)
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review192932

Data review only. r+ with review comments addressed.

These probes are collecting Type 2 data about users interacting with the onboarding overlay and the sessions associated with that. Onboarding telemetry also uses the Mozilla Onyx service for handling the telemetry and selecting content to show, which already being used for Activity Stream as well.

::: browser/extensions/onboarding/README.md:29
(Diff revision 24)
>  
>  ## Architecture
>  
> -Everytime `about:home` or `about:newtab` page is opened, onboarding overlay is injected into that page.
> +![](https://i.imgur.com/7RK89Zw.png)
>  
> -`OnboardingTourType.jsm` will check the onboarding tour type (currently support `new` and `update`). Then in `onboarding.js`, All tours are defined inside of `onboardingTourset` dictionary. `getTourIDList` function will load tours from proper preferences. (Check `How to change the order of tours` section for more detail).
> +During booting from `bootstrap.js`, `OnboardingTourType.jsm` will check the onboarding tour type (currently support `new` and `update`) and set required initial states into preferences.

Generally I discourage "currently" because no one knows when currently is when reading a README, perhaps list this later where you list the onboarding.js file and say "for example, 'new' or 'update'.

::: browser/extensions/onboarding/data_events.md:65
(Diff revision 24)
> +
> +| KEY | DESCRIPTION | &nbsp; |
> +|-----|-------------|:-----:|
> +| `addon_version` | [Required] The version of the Onboarding addon. | :one:
> +| `category` | [Required] Either ("overlay-interactions", "notification-interactions") to identify which kind of the interaction | :one:
> +| `client_id` | [Required] An identifier generated by `ClientID` module to provide an anonymous identifier for this device. Auto append by `ping-centre` module | :one:

I wouldn't use "anonymous" here - that's not correct unless a new id is generated for each ping.

Optionally if you can elaborate, is this id used anywhere else? e.g. by other ping-center modules within Firefox. (Or is this id tied to a Firefox instance, or device and would persist across Firefox uninstall/reinstall)?

::: browser/extensions/onboarding/data_events.md:67
(Diff revision 24)
> +|-----|-------------|:-----:|
> +| `addon_version` | [Required] The version of the Onboarding addon. | :one:
> +| `category` | [Required] Either ("overlay-interactions", "notification-interactions") to identify which kind of the interaction | :one:
> +| `client_id` | [Required] An identifier generated by `ClientID` module to provide an anonymous identifier for this device. Auto append by `ping-centre` module | :one:
> +| `event` | [Required] The type of event. allowed event strings are defined in the below section | :one:
> +| `impression` | [Optional] An integer to record the current notification tour's impression. | :one:

Please enumerate the cases this could be. It's not clear what "impression integer" is.

::: browser/extensions/onboarding/data_events.md:68
(Diff revision 24)
> +| `addon_version` | [Required] The version of the Onboarding addon. | :one:
> +| `category` | [Required] Either ("overlay-interactions", "notification-interactions") to identify which kind of the interaction | :one:
> +| `client_id` | [Required] An identifier generated by `ClientID` module to provide an anonymous identifier for this device. Auto append by `ping-centre` module | :one:
> +| `event` | [Required] The type of event. allowed event strings are defined in the below section | :one:
> +| `impression` | [Optional] An integer to record the current notification tour's impression. | :one:
> +| `ip` | [Auto populated by Onyx] The IP address of the client. | :two:

I would suggest adding a comment here based on :nanj's response (collected with permission, used to create relevant tiles based on inferred geo, and describe the Cloud Services policy for data retention).

::: browser/extensions/onboarding/data_events.md:68
(Diff revision 24)
> +| `addon_version` | [Required] The version of the Onboarding addon. | :one:
> +| `category` | [Required] Either ("overlay-interactions", "notification-interactions") to identify which kind of the interaction | :one:
> +| `client_id` | [Required] An identifier generated by `ClientID` module to provide an anonymous identifier for this device. Auto append by `ping-centre` module | :one:
> +| `event` | [Required] The type of event. allowed event strings are defined in the below section | :one:
> +| `impression` | [Optional] An integer to record the current notification tour's impression. | :one:
> +| `ip` | [Auto populated by Onyx] The IP address of the client. | :two:

I'd recommend adding a comment here based on :nanj's response about Onyx regarding what this is being used for (tile relevance based on inferred geo, is used with permission) and the data retention policies that Cloud Services has. Otherwise it's not necessarily clear why IP address is being collected. I'm also assuming that since this service is used elsewhere in Mozilla for Activity Stream - ideally you can link to those policies too.

::: browser/extensions/onboarding/data_events.md:71
(Diff revision 24)
> +| `event` | [Required] The type of event. allowed event strings are defined in the below section | :one:
> +| `impression` | [Optional] An integer to record the current notification tour's impression. | :one:
> +| `ip` | [Auto populated by Onyx] The IP address of the client. | :two:
> +| `locale` | The browser chrome's language (eg. en-US). | :two:
> +| `page` | [Required] One of ["about:newtab", "about:home"]| :one:
> +| `session_begin` | Time in (integer) milliseconds when onboarding/overlay/notification becoming visible. | :one:

s/time/timestamp to be clearer

::: browser/extensions/onboarding/data_events.md:72
(Diff revision 24)
> +| `impression` | [Optional] An integer to record the current notification tour's impression. | :one:
> +| `ip` | [Auto populated by Onyx] The IP address of the client. | :two:
> +| `locale` | The browser chrome's language (eg. en-US). | :two:
> +| `page` | [Required] One of ["about:newtab", "about:home"]| :one:
> +| `session_begin` | Time in (integer) milliseconds when onboarding/overlay/notification becoming visible. | :one:
> +| `session_end` | Time in (integer) milliseconds when onboarding/overlay/notification losing focus. | :one:

s/time/timestamp to be clearer
Attachment #8897772 - Flags: review?(liuche) → review+
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review184230

> The key point is about what is supposed to be measured. If this ping was to measure the every notification session between start and end, then should be put here. If not, then should not be put here. Would you confirm this?

Not sure if we are measuring the right ping. In the beginning you were uncertain about this and said need a discssion. Then updated the code without explaining if that was desired but only because that was feasible in code. The question what should be measured is just closed and not answered.
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review193424
Attachment #8897772 - Flags: review?(fliu)
(In reply to Fischer [:Fischer] from comment #66)
> Comment on attachment 8897772 [details]
> Bug 1389424 - enable onboarding telemetry via ping-centre;
> 
> https://reviewboard.mozilla.org/r/169080/#review184230
> 
> > The key point is about what is supposed to be measured. If this ping was to measure the every notification session between start and end, then should be put here. If not, then should not be put here. Would you confirm this?
> 
> Not sure if we are measuring the right ping. In the beginning you were
> uncertain about this and said need a discssion. Then updated the code
> without explaining if that was desired but only because that was feasible in
> code. The question what should be measured is just closed and not answered.

Sorry for confuse you. Yes, PM want to measure every notification session between start and end.

In the beginning I mean I'm not sure if `onboarding-notification-close-btn` is the right place for `notification-session-end` tracking point (Rex added in the shield study https://github.com/mozilla/shield-onboarding/blob/master/addon/content/onboarding.js#L515 which does not cover all notification close case). The issue is resolved with current implementation.
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review184230

> Not sure if we are measuring the right ping. In the beginning you were uncertain about this and said need a discssion. Then updated the code without explaining if that was desired but only because that was feasible in code. The question what should be measured is just closed and not answered.

yes, PM want to measure every notification session between start and end
(In reply to Fred Lin [:gasolin] from comment #69)
> Comment on attachment 8897772 [details]
> Bug 1389424 - enable onboarding telemetry via ping-centre;
> 
> https://reviewboard.mozilla.org/r/169080/#review184230
> 
> > Not sure if we are measuring the right ping. In the beginning you were uncertain about this and said need a discssion. Then updated the code without explaining if that was desired but only because that was feasible in code. The question what should be measured is just closed and not answered.
> 
> yes, PM want to measure every notification session between start and end

Sorry Fischer, this message should be posted last week before resend the review. It's finally poped now...
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review191980

> What does CTA stand for?

It's Call-To-Action Button, update the acronym to make it more clear

> Is it possible to get here and this.state.sessions[session_key] is still undefined? Does this event *always* occur after "onboarding-session-begin"?

`onboarding-session-begin` is called before `_initUI`. Since the flow is synchronous, any other UI session always occur after `onboarding-session-begin`

> How come you don't keep "session_id" for notification and overlay? Do all sessions share the same session_id that's set by "onboarding-session-begin"?

All sessions share the same session_id that's set by "onboarding-session-begin". 

The life cycle is

onboarding start
  open notification
  close notification
  open overlay
  close overlay
onboarding closed

so all sessions can share the same session_id to identify they work in the same session.

> How come you don't delete this.state.sessions[session_key] here?

All sessions share the same session_id so we can delete that once at `onboarding-session-end`

> Same here, don't really need locale.

removed, thanks

> locale is actually set by PingCentre by default. So you shouldn't need this.

updated

> Another way you could do this is using the portID attribute that's available as "msg.target.portID" when chrom receives msg from content.
> 
> Activity Stream does this: https://github.com/mozilla/activity-stream/blob/e708b26ebfc03086bb8661a6bda107e77bfd4dee/system-addon/lib/ActivityStreamMessageChannel.jsm#L178

msg.target.portID is not available since onboarding is loaded via framescript instead of the remote page, Fischer's suggestion (use Date.now()) works as well
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review193794

It was a little confusing understanding the session events but it made sense once I tested your patch and looked at the output. Perhaps more details in the comments describing onboarding-session-begin and onboarding-session-end that even if you have not interacted with onboarding at all, these sessions have started when the newtab has loaded.
Attachment #8897772 - Flags: review?(msamuel) → review+
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review193880

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:53
(Diff revision 25)
> +  // track when the overlay is closed
> +  "overlay-session-end": {topic: "firefox-onboarding-session", category: "overlay-interactions"},
> +  // track when click the overlay "skip tour" button
> +  "overlay-skip-tour": {topic: "firefox-onboarding-event", category: "overlay-interactions"},
> +};
> +const ICON_STATE_DEFAULT = "default";

Use `ONBOARDING_STATE_DEFAULT` because it in fact corresponds to the "browser.onboarding.state" pref

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:81
(Diff revision 25)
> +    }
> +
> +    switch (event) {
> +      case "onboarding-session-begin":
> +        // track the tab session and fill related data
> +        if (session_key) {

Need to throw at no `session_key` as well because all cases here plus inside `send` method all rely on `session_key`. `session_key` is an important must as `topic`.

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:83
(Diff revision 25)
> +    switch (event) {
> +      case "onboarding-session-begin":
> +        // track the tab session and fill related data
> +        if (session_key) {
> +          if (!this.state.sessions[session_key]) {
> +            this.state.sessions[session_key] = {};

This `if (!this.state.sessions[session_key]) this.state.sessions[session_key] = {};` should go before `switch` because:
1. `this.state.sessions[session_key]` is required on all cases
2. although "onboarding-session-begin" case should be the 1st ping, we cannot see this from OnboardingTelemetry.jsm so it shouldn't be relyied only on "onboarding-session-begin" case

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:85
(Diff revision 25)
> +        // track the tab session and fill related data
> +        if (session_key) {
> +          if (!this.state.sessions[session_key]) {
> +            this.state.sessions[session_key] = {};
> +          }
> +          this.state.sessions[session_key].session_id = String(gUUIDGenerator.generateUUID());

Use `gUUIDGenerator.generateUUID().toString()`. Probably better to rely on its internal toString method.

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:100
(Diff revision 25)
> +      case "notification-session-begin":
> +        if (session_key) {
> +          this.state.sessions[session_key].notification_session_begin = Date.now();
> +        }
> +        break;
> +      default:

Use 
```
case "overlay-session-end":
case "onboarding-session-end":
case "notification-session-end":
```
instead of
```
default:
```
because in fact it is quite important that here only handles some specific cases not in general

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:107
(Diff revision 25)
> +        break;
> +    }
> +  },
> +
> +  // send out pings by topic
> +  send(topic, data) {

Rename to `_send` to let other know shouldn't call it directly in the normal usage. Many data it needed, such as `state.sessions[session_key]`, `xxx_session_begin` rely on `process` to process.

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:145
(Diff revision 25)
> +            break;
> +        }
> +
> +        // the field is used to identify how user open the overlay (through default logo or watermark),
> +        // the number of open from notification can be retrieved via `notification-cta-click` event
> +        const tour_source = Services.prefs.getStringPref("browser.onboarding.state", ICON_STATE_DEFAULT);

From the doc[3], the `tour_source` has 3 types: default, watermark, notification and exists in the "overlay-session" [1]. The "notification-session", for instance, should carry an empty `tour_source`[2]. But this only sends default, watermark and sends on every session. Did you talk to Product/UX about this change?

[1]
```
{
"event": "overlay-session",
"addon_version": "1.0.12",
"client_id": "26288a14-5cc4-d14f-ae0a-bb01ef45be9c",
"locale": "en-US",
"page": ["about:newtab" | "about:home"],
"session_id": "005deed0-e3e4-4c02-a041-17405fd703f6",
"session_begin": 4199,
"session_end": 4199,
“tour_source”: [default|watermark|notification],
"category": "overlay-interactions"
“topic”: “firefox-onboarding-session”
}
```

[2]
```
{
"event": "notification-session",
"addon_version": "1.0.12",
"client_id": "26288a14-5cc4-d14f-ae0a-bb01ef45be9c",
"locale": "en-US",
"page": ["about:newtab" | "about:home"],
"session_id": "005deed0-e3e4-4c02-a041-17405fd703f6",
"session_begin": 4199,
"session_end": 4199,
“tour_source”: ””,
"tour_id": ["private"|"sync"|...],
"impression": : 1 (number < 8)
"category": "notification-interactions",
“topic”: “firefox-onboarding-session”
}

```

[3] https://docs.google.com/document/d/1hc6RLoWglQ58Uq4n9ofjMLhwTSWQcs7q9K4AAmjk-ow/edit#

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:151
(Diff revision 25)
> +        const session_end = Date.now();
> +        this.sessionProbe && this.sessionProbe.sendPing({
> +          addon_version,
> +          category,
> +          event,
> +          page,

Looks like `page` is an important must for every ping so that Product/UX could utilize it to analyze. See the "onboarding-session-begin" telemetry in onboarding.js for further discussion.

::: browser/extensions/onboarding/bootstrap.js:78
(Diff revision 25)
>        case PREF_STRING:
>          Services.prefs.setStringPref(name, pref.value);
>          break;
>  
>        default:
>          throw new TypeError(`Unexpected type (${type}) for preference ${name}.`)

Would you help to add `;` in the end btw, thanks for improving the code.

::: browser/extensions/onboarding/bootstrap.js:160
(Diff revision 25)
>          msg.target.messageManager.sendAsyncMessage("Onboarding:ResponseLoginStatus", {
>            isLoggedIn: syncTourChecker.isLoggedIn()
>          });
>          break;
> +      case "ping-centre":
> +        OnboardingTelemetry.process(msg.data.params.data);

Calling `process` might throw. We need try...catch(e) { Cu.reportError(e); }

::: browser/extensions/onboarding/content/onboarding.js:398
(Diff revision 25)
>      this._resizeTimerId =
>        this._window.requestIdleCallback(() => this._resizeUI());
> +    telemetry({
> +      event: "onboarding-session-begin",
> +      page: this._window.location.href,
> +      session_key: this._session_key,

Since `session_key` and `page` are both important must to have OnboardingTelemetry.jsm functions well. Simply make these 2 passed to OnboardingTelemetry.jsm btw in the 1st ping it too implicit and error-prone. What if someday someone remove/modity this ping, then everything would be broken. A function, such as `registerTelemetry` or `initTelemetry` or ..., which will register `session_key` and `page` to OnboardingTelemetry.jsm is required. So in while reading `init` codes then we will know some prerequisite data/setup is required for telemetry. It will not get ignored or dropped easily.

::: browser/extensions/onboarding/content/onboarding.js:818
(Diff revision 25)
>          params.push({
>            name: `browser.onboarding.tour.${id}.completed`,
>            value: true
>          });
> +        telemetry({
> +          event: "overlay-cta-click",

This "overlay-cta-click" shouldn't be here because some tour is set to completed without needing cta button clicked and skipping tour will call `setToursCompleted` too
Attachment #8897772 - Flags: review?(fliu)
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review193880

> Need to throw at no `session_key` as well because all cases here plus inside `send` method all rely on `session_key`. `session_key` is an important must as `topic`.

updated

> This `if (!this.state.sessions[session_key]) this.state.sessions[session_key] = {};` should go before `switch` because:
> 1. `this.state.sessions[session_key]` is required on all cases
> 2. although "onboarding-session-begin" case should be the 1st ping, we cannot see this from OnboardingTelemetry.jsm so it shouldn't be relyied only on "onboarding-session-begin" case

updated

> Use `gUUIDGenerator.generateUUID().toString()`. Probably better to rely on its internal toString method.

searched and found only AS use the syntax `String(gUUIDGenerator.generateUUID())`, update to use `toString()`

> Use 
> ```
> case "overlay-session-end":
> case "onboarding-session-end":
> case "notification-session-end":
> ```
> instead of
> ```
> default:
> ```
> because in fact it is quite important that here only handles some specific cases not in general

only `topic: "internal"` events should process here. Every other events should send the ping, so we don't need to list them all here.

> From the doc[3], the `tour_source` has 3 types: default, watermark, notification and exists in the "overlay-session" [1]. The "notification-session", for instance, should carry an empty `tour_source`[2]. But this only sends default, watermark and sends on every session. Did you talk to Product/UX about this change?
> 
> [1]
> ```
> {
> "event": "overlay-session",
> "addon_version": "1.0.12",
> "client_id": "26288a14-5cc4-d14f-ae0a-bb01ef45be9c",
> "locale": "en-US",
> "page": ["about:newtab" | "about:home"],
> "session_id": "005deed0-e3e4-4c02-a041-17405fd703f6",
> "session_begin": 4199,
> "session_end": 4199,
> “tour_source”: [default|watermark|notification],
> "category": "overlay-interactions"
> “topic”: “firefox-onboarding-session”
> }
> ```
> 
> [2]
> ```
> {
> "event": "notification-session",
> "addon_version": "1.0.12",
> "client_id": "26288a14-5cc4-d14f-ae0a-bb01ef45be9c",
> "locale": "en-US",
> "page": ["about:newtab" | "about:home"],
> "session_id": "005deed0-e3e4-4c02-a041-17405fd703f6",
> "session_begin": 4199,
> "session_end": 4199,
> “tour_source”: ””,
> "tour_id": ["private"|"sync"|...],
> "impression": : 1 (number < 8)
> "category": "notification-interactions",
> “topic”: “firefox-onboarding-session”
> }
> 
> ```
> 
> [3] https://docs.google.com/document/d/1hc6RLoWglQ58Uq4n9ofjMLhwTSWQcs7q9K4AAmjk-ow/edit#

Yes, I talked to Cindy that we can log logo/watermark as tour_source. For overlay opened from the notification, it can be counted by `notification_cta_click` event.

> Looks like `page` is an important must for every ping so that Product/UX could utilize it to analyze. See the "onboarding-session-begin" telemetry in onboarding.js for further discussion.

I'm not sure what issue is within this part?

> Since `session_key` and `page` are both important must to have OnboardingTelemetry.jsm functions well. Simply make these 2 passed to OnboardingTelemetry.jsm btw in the 1st ping it too implicit and error-prone. What if someday someone remove/modity this ping, then everything would be broken. A function, such as `registerTelemetry` or `initTelemetry` or ..., which will register `session_key` and `page` to OnboardingTelemetry.jsm is required. So in while reading `init` codes then we will know some prerequisite data/setup is required for telemetry. It will not get ignored or dropped easily.

created `registerNewTelemetrySession` alias in onboarding.js and handle the init case as a separate `onboarding-register-session` internal event.

> This "overlay-cta-click" shouldn't be here because some tour is set to completed without needing cta button clicked and skipping tour will call `setToursCompleted` too

Nice catch! Now call when click `onboarding-tour-action-button` or when tagged completed instantly upon showing
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review194710

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:80
(Diff revision 27)
> +    let topic = EVENT_WHITELIST[event] && EVENT_WHITELIST[event].topic;
> +    if (!topic) {
> +      throw new Error(`ping-centre doesn't know ${event}, only knows ${Object.keys(EVENT_WHITELIST)}`);
> +    }
> +
> +    if (!this.state.sessions[session_key]) {

What if `session_key` is undefined here? We should check `session_key` here.

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:100
(Diff revision 27)
> +      case "onboarding-session-begin":
> +        // track the tab session and fill related data
> +        if (session_key) {
> +          this.state.sessions[session_key].onboarding_session_begin = Date.now();
> +        } else {
> +          throw new Error(`session_key is required for ${event}`);

This(and others) should be moved before switch

::: browser/extensions/onboarding/content/onboarding.js:816
(Diff revision 27)
>        case "onboarding-tour-default-browser":
>        case "onboarding-tour-sync":
>        case "onboarding-tour-performance":
>          this.setToursCompleted([tourId]);
> +        telemetry({
> +          event: "overlay-cta-click",

So this means we still measure this as a cta button click even no cta button is clicked. If have confirmed with Product/UX, that is fine.
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review194730

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:178
(Diff revision 27)
> +          tour_id,
> +          tour_source,
> +        }, {filter: ONBOARDING_ID});
> +        break;
> +      case "firefox-onboarding-event":
> +        let impression = Services.prefs.getIntPref(PROMPT_COUNT_PREF, 0);

Per the offline talk, send `impression = -1` for pings unrelated to tour notification
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review193880

> only `topic: "internal"` events should process here. Every other events should send the ping, so we don't need to list them all here.

OK then please do
```
if (topic == "internal") {
  switch (event) {
    case "onboarding-register-session":
     ... ...
     
    case "onboarding-session-begin":
     ... ...
  }
} else {
  this._send(topic, data);
}
```
to make it explicit that here is handling the "internal" topic

> I'm not sure what issue is within this part?

This is about `registerNewTelemetrySession`
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review194738

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:156
(Diff revision 27)
> +            event = "overlay-session";
> +            session_begin = overlay_session_begin;
> +            break;
> +          case "notification-session-end":
> +            event = "notification-session";
> +            session_begin = notification_session_begin;

What if no `session_begin`? Should it throw?
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review194750

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:118
(Diff revision 27)
> +        } else {
> +          throw new Error(`session_key is required for ${event}`);
> +        }
> +        break;
> +      default:
> +        this._send(topic, data);

OK then please do
```
if (topic == "internal") {
  switch (event) {
    case "onboarding-register-session":
     ... ...
     
    case "onboarding-session-begin":
     ... ...
  }
} else {
  this._send(topic, data);
}
```
to make it explicit that here is handling the "internal" topic
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review184230

> yes, PM want to measure every notification session between start and end

Just talked to PM: Cindy, we need to measure notification-close-button-click ping
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review194758
Attachment #8897772 - Flags: review?(fliu)
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review194760

::: browser/extensions/onboarding/content/onboarding.js:552
(Diff revision 27)
>        // that means clicking outside the tour content area.
>        // Let's toggle the overlay.
>        case "onboarding-overlay":
>          this.hideOverlay();
>          break;
>        case "onboarding-notification-close-btn":

Just talked to PM: Cindy, we need to measure notification-close-button-click ping
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review192932

> I wouldn't use "anonymous" here - that's not correct unless a new id is generated for each ping.
> 
> Optionally if you can elaborate, is this id used anywhere else? e.g. by other ping-center modules within Firefox. (Or is this id tied to a Firefox instance, or device and would persist across Firefox uninstall/reinstall)?

"anonymous" Removed. I haven't dig into the details of ClientID module, but will provide the source link in doc for reference https://github.com/mozilla/gecko-dev/blob/master/toolkit/modules/ClientID.jsm

> Please enumerate the cases this could be. It's not clear what "impression integer" is.

updated

> I would suggest adding a comment here based on :nanj's response (collected with permission, used to create relevant tiles based on inferred geo, and describe the Cloud Services policy for data retention).

Added nan's explanation from comment 46
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review194710

> So this means we still measure this as a cta button click even no cta button is clicked. If have confirmed with Product/UX, that is fine.

Cindy said she is fine either send or not send the event here. But I think we should track all `set tour complete` case to make sure tours get the fair comparism data. (and it will be easier to filter set complete data with the same `overlay-cta-click` event)
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review184818

> This still may have collision since the purpose is really for id. Why not use timestamp?

timestamp makes sense, update
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review194802

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:116
(Diff revision 28)
> +          break;
> +      }
> +    } else {
> +      switch (event) {
> +        case "onboarding-session-end":
> +          if (!this.state.sessions[session_key].onboarding_session_begin) {

Why not check this inside `_send`?

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:153
(Diff revision 28)
> +      overlay_session_begin,
> +      page,
> +      session_id,
> +    } = this.state.sessions[session_key];
> +    let category = EVENT_WHITELIST[event].category;
> +    let session_begin;

Move `let session_begin;` down into the case using it

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:192
(Diff revision 28)
> +        }, {filter: ONBOARDING_ID});
> +        break;
> +      case "firefox-onboarding-event":
> +        let impression = (event === "notification-close-button-click" ||
> +          event === "notification-cta-click") ?
> +          Services.prefs.getIntPref(PROMPT_COUNT_PREF, 0) : -1;

I would say simply "browser.onboarding.notification.prompt-count" here is enough(like above "browser.onboarding.state"), only here is using PROMPT_COUNT_PREF. Up to you.
Attachment #8897772 - Flags: review?(fliu)
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review194826

::: browser/extensions/onboarding/content/onboarding.js:822
(Diff revision 28)
>        case "onboarding-tour-default-browser":
>        case "onboarding-tour-sync":
>        case "onboarding-tour-performance":
>          this.setToursCompleted([tourId]);
> +        telemetry({
> +          event: "overlay-cta-click",

Add comments to explain why count "overlay-cta-click" as well since this is really a special case.
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review194802

> Why not check this inside `_send`?

updated

> Move `let session_begin;` down into the case using it

There're 3 cases (onboarding/overlay/notification-session-end) will assign `session_begin`, so its better define at top.

> I would say simply "browser.onboarding.notification.prompt-count" here is enough(like above "browser.onboarding.state"), only here is using PROMPT_COUNT_PREF. Up to you.

updated
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review194826

> Add comments to explain why count "overlay-cta-click" as well since this is really a special case.

added
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review195280

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:88
(Diff revision 30)
> +    }
> +
> +    if (!this.state.sessions[session_key]) {
> +      this.state.sessions[session_key] = {};
> +    }
> +

Per the offline discussin, add such as
```
registerNewSession(session_key, page) {
 if (this.state.sessions[session_key]) return;
 if (!session_key || !page) throw;
 this.state.sessions[session_key] = {};
 this.state.sessions[session_key].session_id =
    gUUIDGenerator.generateUUID().toString();
}
```

then do in `process` after `if (!topic) ...`
```
if (event == "onboarding-register-session") this.registerNewSession(session_key, page);

if (!this.state.sessions[session_key]) throw;

if (topic === "internal") ... ...
```

::: browser/extensions/onboarding/data_events.md:20
(Diff revision 30)
> +```js
> +{
> +  // These fields are sent from the client
> +  "addon_version": "1.0.0",
> +  "category": ["overlay-interactions"|"notification-interactions"],
> +  "client_id": "374dc4d8-0cb2-4ac5-a3cf-c5a9bc3c602e",

Do we need to send "client_id"? Here says "sent from the client" but in OnboardingTelemetry.jsm we never send client_id.

::: browser/extensions/onboarding/data_events.md:21
(Diff revision 30)
> +{
> +  // These fields are sent from the client
> +  "addon_version": "1.0.0",
> +  "category": ["overlay-interactions"|"notification-interactions"],
> +  "client_id": "374dc4d8-0cb2-4ac5-a3cf-c5a9bc3c602e",
> +  "locale": "en-US",

In OnboardingTelemetry.jsm, locale is never sent but here plus talking to PM, we need it for analysis. Call `Services.locale.getRequestedLocale()` to get Firefox's current lang and send it back to the server.

::: browser/extensions/onboarding/data_events.md:75
(Diff revision 30)
> +| `page` | [Required] One of ["about:newtab", "about:home"]| :one:
> +| `session_begin` | Timestamp in (integer) milliseconds when onboarding/overlay/notification becoming visible. | :one:
> +| `session_end` | Timestamp in (integer) milliseconds when onboarding/overlay/notification losing focus. | :one:
> +| `session_id` | [Required] The unique identifier generated by `gUUIDGenerator` service to identify the specific user session when onboarding is inited/when overlay is opened/when notification is shown. | :one:
> +| `tour_source` | Either ("default", "watermark") indicates the overlay is opened while in the default or the watermark state. Open from the notification bar is counted via 'notification-cta-click event'. | :two:
> +| `tour_id` | Either ("default", "watermark") indicates how the overlay is triggered. The number of open from notification can be retrieved via 'notification-cta-click event'. | :two:

We got "| `tour_source` | Either ("default", "watermark")" and "| `tour_id` | Either ("default", "watermark")" which both are the same??

::: browser/extensions/onboarding/data_events.md:75
(Diff revision 30)
> +| `page` | [Required] One of ["about:newtab", "about:home"]| :one:
> +| `session_begin` | Timestamp in (integer) milliseconds when onboarding/overlay/notification becoming visible. | :one:
> +| `session_end` | Timestamp in (integer) milliseconds when onboarding/overlay/notification losing focus. | :one:
> +| `session_id` | [Required] The unique identifier generated by `gUUIDGenerator` service to identify the specific user session when onboarding is inited/when overlay is opened/when notification is shown. | :one:
> +| `tour_source` | Either ("default", "watermark") indicates the overlay is opened while in the default or the watermark state. Open from the notification bar is counted via 'notification-cta-click event'. | :two:
> +| `tour_id` | Either ("default", "watermark") indicates how the overlay is triggered. The number of open from notification can be retrieved via 'notification-cta-click event'. | :two:

In OnboardingTelemetry.jsm, some ping will send a "" tour_id. I guess this is acceptable. If my guess is right, please document that we will send a "" tour_id back when it is irrelavant to thta ping, like the `impression = -1` documentation.

::: browser/extensions/onboarding/data_events.md:75
(Diff revision 30)
> +| `page` | [Required] One of ["about:newtab", "about:home"]| :one:
> +| `session_begin` | Timestamp in (integer) milliseconds when onboarding/overlay/notification becoming visible. | :one:
> +| `session_end` | Timestamp in (integer) milliseconds when onboarding/overlay/notification losing focus. | :one:
> +| `session_id` | [Required] The unique identifier generated by `gUUIDGenerator` service to identify the specific user session when onboarding is inited/when overlay is opened/when notification is shown. | :one:
> +| `tour_source` | Either ("default", "watermark") indicates the overlay is opened while in the default or the watermark state. Open from the notification bar is counted via 'notification-cta-click event'. | :two:
> +| `tour_id` | Either ("default", "watermark") indicates how the overlay is triggered. The number of open from notification can be retrieved via 'notification-cta-click event'. | :two:

In OnboardingTelemetry.jsm, some ping send a "" tour_id. I guess this is acceptable. If my guess is correct, document that a "" tour_id will be sent when tour_id is irrelevant to that ping, like the "imperssion = -1" documentation.
Attachment #8897772 - Flags: review?(fliu)
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review194802

> There're 3 cases (onboarding/overlay/notification-session-end) will assign `session_begin`, so its better define at top.

I meant after `case "firefox-onboarding-session":` but no strong opinion on it up to you.
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review195280

> Do we need to send "client_id"? Here says "sent from the client" but in OnboardingTelemetry.jsm we never send client_id.

client_id is auto included by PingCentre module

> In OnboardingTelemetry.jsm, locale is never sent but here plus talking to PM, we need it for analysis. Call `Services.locale.getRequestedLocale()` to get Firefox's current lang and send it back to the server.

As comment 60, Locale is auto-included by pingCentre module

> We got "| `tour_source` | Either ("default", "watermark")" and "| `tour_id` | Either ("default", "watermark")" which both are the same??

Thanks for catch that. It's a wrong place description. tour_id means `id of the current tour`
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

https://reviewboard.mozilla.org/r/169080/#review195374
Attachment #8897772 - Flags: review?(fliu) → review+
Thanks for review, will land after tree green
Flags: needinfo?(msamuel)
Component: New Tab Page → Tours
(rebase to seek for a more stable base)
Pushed by flin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c94d9d435a0d
enable onboarding telemetry via ping-centre;r=emtwo,Fischer,liuche
Pushed by flin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/31d0a5935711
enable onboarding telemetry via ping-centre;r=emtwo,Fischer,liuche
Thanks! Lint fixed. I've rebased to latest central in previous PR. Not sure why ES lint check is missing in tree herder...
Flags: needinfo?(gasolin)
https://hg.mozilla.org/mozilla-central/rev/31d0a5935711
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: Firefox 57 → Firefox 58
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

Approval Request Comment
[Feature/Bug causing the regression]: bug 1388648
[User impact if declined]: can't get telemetry result of user activity for onboarding
[Is this code covered by automated tests?]: No new test is added but onboarding test suite will fail if this change broke. Its also being tested manually by several reviewers.
[Has the fix been verified in Nightly?]: There's no user aware activity for telemetry so this don't need QA verify.
[Needs manual test from QE? If yes, steps to reproduce]: N
[List of other uplifts needed for the feature/fix]: N
[Is the change risky?]: N
[Why is the change risky/not risky?]: Most code are called within a try sentence so any potential fail won't effect the main behavior. And test suite will fail if this doesn't work.
[String changes made/needed]: N
Attachment #8897772 - Flags: approval-mozilla-beta?
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

I would rather have this ride the train with 58. Especially when I see that it landed in nightly only 7 hours and the patch is pretty big.
Attachment #8897772 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
As comment 63 the patch will give us data that would help the team iterate on the onboarding experience.

Note in this patch, about 120+ lines are just documents, and ~60 lines are adding telemetry points. 
The rest ~200 lines changes are load from a separate file, which wrapped through a try/catch clause, that means any error will be caught and not affect the main onboarding activity.

Nicole, do you have anything to add?
Flags: needinfo?(nyee)
Sylvestre and I discussed this one. This is planned work for Onboarding Tours in 57. This should be in b11.
status-firefox57: wontfix → affected
Comment on attachment 8897772 [details]
Bug 1389424 - enable onboarding telemetry via ping-centre;

This is a must, beta57+
Attachment #8897772 - Flags: approval-mozilla-beta- → approval-mozilla-beta+
Flags: needinfo?(nyee)
Blocks: 1412164
Blocks: 1412255
Blocks: 1412257
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: