Closed Bug 1413830 Opened 6 years ago Closed 6 years ago

Data schema change for onboarding telemetry

Categories

(Firefox :: Tours, enhancement, P1)

Product:
Firefox
Component:
Tours
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: gasolin, Assigned: gasolin)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [onboarding-telemetry])

Attachments

(3 files, 5 obsolete files)

Bug 1413830 - Data schema change for onboarding telemetry;
59 bytes, text/x-review-board-request
francois
: review+
Fischer
: review+
Details
data-review-request
4.49 KB, text/markdown
francois
: review+
Details
Bug-1413830-Data-schema-change-for-onboarding-telemetry.patch
46.77 KB, patch
Details | Diff | Splinter Review 
Redesign of onboarding telemetry tables

We want to design for more clear sessions reference and events at once
Whiteboard: [onboarding-telemetry] → [onboarding-telemetry][triage]
Whiteboard: [onboarding-telemetry][triage] → [onboarding-telemetry]
Attachment #8928041 - Attachment is obsolete: true
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

Hi this is a 3 part patch that fullfil the new telemetry schema:

Part 1. add new telemetry events (open/close/click..)

Part 2. properly handle sessions (right time to start, store and send root/parent sessions)

Part 3. add notification_state


Not included:

* send bubble_state
* send onboarding-noshow/overlay-disapear event and send the width
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review204388

Could we hold on for a little while since a discussion with Product is near tomorrow?
Attachment #8927140 - Flags: review?(fliu)
Comment on attachment 8927141 [details]
Bug 1413830 - PART 2: properly handle sessions for onboarding telemetry;

https://reviewboard.mozilla.org/r/198386/#review204392
Attachment #8927141 - Flags: review?(fliu)
Comment on attachment 8927193 [details]
Bug 1413830 - PART 3: send notification_state;

https://reviewboard.mozilla.org/r/198462/#review204394
Attachment #8927193 - Flags: review?(fliu)
Blocks: 1412255
Attachment #8927193 - Attachment is obsolete: true
Blocks: 1417769
Status: NEW → ASSIGNED
Priority: P3 → P1
Target Milestone: --- → Firefox 59
The schema is not changed after discuss with PM yesterday. Please help review it, thanks.

I've move PART3 work into bug 1412255 so we can focus on new events and sessions in this bug.

* send bubble_state
will deal in bug 1412255 as well

* send onboarding-noshow/overlay-disapear event and send the width
will deal in bug 1417769
Hi, we plan to update our onboarding telemetry for v59 and want to have a new table

In the patch we will send `firefox-onboarding-session2/firefox-onboarding-event2` instead of `firefox-onboarding-session/firefox-onboarding-event` as topic. Is the change sufficient for log events to a new table?


Here are the current designed columns
https://docs.google.com/spreadsheets/d/1BQzncwklWZvvkLtbLaYO2unMG3Tw9LQX-c6XXqmYTUo/edit#gid=1206558344

Most of columns are the same as the previous one, but I need your help to check if any column name doesn't fit the server requirement.
Ex: is it ok to use 'type' instead of 'event' as a column name?
Flags: needinfo?(najiang)
I guess we need data-review as well??
Flags: needinfo?(gasolin)
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review205272

Just realized, currently it is very eazy to miss which ping param is required for every events and which is optional. Some mechanism is needed to make sure: (1) data sanitization (2) correct ping params sent and (3) report error when seeing incorrect params come.

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:185
(Diff revision 7)
>    _send(topic, data) {
>      let {
>        addon_version,
>      } = this.state;
>      let {
> +      current_tour_id,

The default value is gone: `current_tour_id = ""` ??

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:188
(Diff revision 7)
>      let {
> +      current_tour_id,
>        event,
> -      tour_id = "",
>        session_key,
> +      target_tour_id,

The default value is gone??

::: browser/extensions/onboarding/data_events.md:93
(Diff revision 7)
>  | `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 the overlay is opened/when notification is shown. | :one:
>  | `timestamp` | Timestamp in (integer) milliseconds when the event triggered | :one:
> -| `tour_id` | id of the current tour. The number of open from notification can be retrieved via 'notification-cta-click event'. We put ` ` when this field is not relevant to this event | :one:
> +| `current_tour_id` | id of the current tour. The number of open from notification can be retrieved via 'notification-cta-click event'. We put ` ` when this field is not relevant to this event | :one:

nit: To align with the real default value. could we say: ".... We put "" when this field is not relevant to this event"

::: browser/extensions/onboarding/data_events.md:94
(Diff revision 7)
>  | `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 the overlay is opened/when notification is shown. | :one:
>  | `timestamp` | Timestamp in (integer) milliseconds when the event triggered | :one:
> -| `tour_id` | id of the current tour. The number of open from notification can be retrieved via 'notification-cta-click event'. We put ` ` when this field is not relevant to this event | :one:
> +| `current_tour_id` | id of the current tour. The number of open from notification can be retrieved via 'notification-cta-click event'. We put ` ` when this field is not relevant to this event | :one:
> +| `target_tour_id` | id of the target switched tour. The number of open from notification can be retrieved via 'notification-cta-click event'. We put ` ` when this field is not relevant to this event | :one:

nit: To align with the real default value. could we say: ".... We put "" when this field is not relevant to this event"

::: browser/extensions/onboarding/data_events.md:121
(Diff revision 7)
>  
> +### Onboarding events
> +
> +| EVENT | DESCRIPTION |
> +|-----------|---------------------|
> +| `onboarding-logo-click` | triggered when user open the overlay via click the logo or the watermark. |

nit: could we say "triggered when user opens the overlay via clicking the logo."? Although the logo == the watermark, in case others unfamiliar with the Onboarding tour might think there were 2 stuffs to click

::: browser/extensions/onboarding/data_events.md:141
(Diff revision 7)
>  
>  ### Notification events
>  
>  | EVENT | DESCRIPTION |
>  |-----------|---------------------|
> +| `notification-appear` | event is sent when user open the notification. |

nit: User cannot open any notification so maybe just like its name: "event is sent when one notification appears"
Attachment #8927140 - Flags: review?(fliu)
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review205284

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:185
(Diff revision 7)
>    _send(topic, data) {
>      let {
>        addon_version,
>      } = this.state;
>      let {
> +      current_tour_id,

The server will put the default value "" when they didn't receive related column, so we can save some bandwidth if the value is not relevant to this event. 
We can send all columns with default value though.
(In reply to Fred Lin [:gasolin] from comment #43)
> Comment on attachment 8927140 [details]
> Bug 1413830 - PART 1: track new events for onboarding telemetry;
> 
> https://reviewboard.mozilla.org/r/198384/#review205284
> 
> ::: browser/extensions/onboarding/OnboardingTelemetry.jsm:185
> (Diff revision 7)
> >    _send(topic, data) {
> >      let {
> >        addon_version,
> >      } = this.state;
> >      let {
> > +      current_tour_id,
> 
> The server will put the default value "" when they didn't receive related
> column, so we can save some bandwidth if the value is not relevant to this
> event. 
> We can send all columns with default value though.
First, how much bandwidth an "" plus 1 or 2 words could occupy? 
Second what if the server side changed? what if some day we need to move to another telemetry system?
Is our code robust enough? Is our code clear enough to reflex requirements of pings?
We can do that not sending these optional value. But, the first thing, making the data examination explicit and clear is important to the future maintenance and correctness.
Comment on attachment 8927141 [details]
Bug 1413830 - PART 2: properly handle sessions for onboarding telemetry;

https://reviewboard.mozilla.org/r/198386/#review205302

Part 2 is like Part 1, needing a mechanism to make sure the correctness of session pings
Attachment #8927141 - Flags: review?(fliu)
Here's the data-review request

1) What questions will you answer with this data?

To analysis user interaction with onboarding tool for better browser retention.

2) Why does Mozilla need to answer these questions?  Are there benefits for users? Do we need this information to address product or business requirements?

We need this information to address product requirements. Data will be used for pursuing better browser retention rate.

3) What alternative methods did you consider to answer these questions? Why were they not sufficient?

We can use current overlay-nav-click and overlay-session event, but `overlay-nav-click` can't identify the user actually clicks the nav button or the user just see the tour because the tour showed to them by default.
overlay-session event can identify user open/close the overlay but can't identify how the user opens/close the overlay.

4) Can current instrumentation answer these questions?

Part of but not very accurate

5) List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox [data c](https://wiki.mozilla.org/Firefox/Data_Collection)[ollection ](https://wiki.mozilla.org/Firefox/Data_Collection)[categories](https://wiki.mozilla.org/Firefox/Data_Collection) on the found on the Mozilla wiki.   

<table>
  <tr>
    <td>Measurement Description</td>
    <td>Data Collection Category</td>
    <td>Tracking Bug #</td>
  </tr>
  <tr>
    <td>onboarding-logo-click triggered when user open the overlay via click the logo or the watermark</td>
    <td>Interaction data</td>
    <td>Bug 1413830</td>
  </tr>
  <tr>
    <td>overlay-current-tour event is sent when any tour is going to display on the overlay</td>
    <td>Interaction data</td>
    <td>Bug 1413830</td>
  </tr>
  <tr>
    <td>overlay-close-button-click triggered when user close the overlay via click the overlay close button</td>
    <td>Interaction data</td>
    <td>Bug 1413830</td>
  </tr>
  <tr>
    <td>overlay-close-outside-click triggered when user close the overlay via click outside of the tour content area</td>
    <td>Interaction data</td>
    <td>Bug 1413830</td>
  </tr>
  <tr>
    <td>notification-appear triggered when one notification appears</td>
    <td>Interaction data</td>
    <td>Bug 1413830</td>
  </tr>
  <tr>
    <td>rename event, tour_source, impression column to type, logo_state, notification_impression to help analyst better understand its purpose</td>
    <td>Interaction data</td>
    <td>Bug 1413830</td>
  </tr>
  <tr>
    <td>log separate root_session_id / parent_session_id so analyst can better identify the event comes from which overlay-session </td>
    <td>Interaction data</td>
    <td>Bug 1413830 PART 2 patch</td>
  </tr>
</table>

Here's the proposed Telemetry events/column mapping
https://docs.google.com/spreadsheets/d/1BQzncwklWZvvkLtbLaYO2unMG3Tw9LQX-c6XXqmYTUo/edit#gid=1206558344

6) How long will this data be collected?  Choose one of the following:

* I want to permanently monitor this data. (Fred Lin)

7) What populations will you measure?

* Which release channels? All

* Which countries? All

* Which locales? All

* Any other filters?  Please describe in detail below.
No

8) Please provide a general description of how you will analyze this data.

Identify how user open/close the overlay and click the tour

9) Where do you intend to share the results of your analysis?

The data will be shared between PM & UX to drive the better decision
Flags: needinfo?(gasolin) → needinfo?(liuche)
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review205272

> The default value is gone: `current_tour_id = ""` ??

added default value

> The default value is gone??

added default value

> nit: To align with the real default value. could we say: ".... We put "" when this field is not relevant to this event"

updated

> nit: To align with the real default value. could we say: ".... We put "" when this field is not relevant to this event"

updated

> nit: could we say "triggered when user opens the overlay via clicking the logo."? Although the logo == the watermark, in case others unfamiliar with the Onboarding tour might think there were 2 stuffs to click

updated

> nit: User cannot open any notification so maybe just like its name: "event is sent when one notification appears"

updated
(In reply to Fred Lin [:gasolin] from comment #40)

> Here are the current designed columns
> https://docs.google.com/spreadsheets/d/1BQzncwklWZvvkLtbLaYO2unMG3Tw9LQX-
> c6XXqmYTUo/edit#gid=1206558344
> 
> Most of columns are the same as the previous one, but I need your help to
> check if any column name doesn't fit the server requirement.
> Ex: is it ok to use 'type' instead of 'event' as a column name?

Given that most of the columns are same, I'd recommend to modify the existing tables by using add/rename/drop columns instead of creating two new tables. Just let the ETL jobs do the column mapping to handle both old/new pings. I've skimmed the schema, looks like that's totally doable.

What do you think?
Flags: needinfo?(najiang)
Depends on: 1418167
(In reply to Nan Jiang [:nanj] from comment #48)
> (In reply to Fred Lin [:gasolin] from comment #40)
> 
> > Here are the current designed columns
> > https://docs.google.com/spreadsheets/d/1BQzncwklWZvvkLtbLaYO2unMG3Tw9LQX-
> > c6XXqmYTUo/edit#gid=1206558344
> > 
> > Most of columns are the same as the previous one, but I need your help to
> > check if any column name doesn't fit the server requirement.
> > Ex: is it ok to use 'type' instead of 'event' as a column name?
> 
> Given that most of the columns are same, I'd recommend to modify the
> existing tables by using add/rename/drop columns instead of creating two new
> tables. Just let the ETL jobs do the column mapping to handle both old/new
> pings. I've skimmed the schema, looks like that's totally doable.
> 
> What do you think?
Hi Jiang,
Could we still create new tables?
This change in fact is not backward compatible.
We have redefined the meanings of some columns or some valid data values of some columns.
Thank you.
Flags: needinfo?(najiang)
(In reply to Fischer [:Fischer] from comment #49)
> (In reply to Nan Jiang [:nanj] from comment #48)
> > (In reply to Fred Lin [:gasolin] from comment #40)
> > 

> Could we still create new tables?
> This change in fact is not backward compatible.
> We have redefined the meanings of some columns or some valid data values of
> some columns.
> Thank you.

Surely you can.

:gasolin, let's use two new ping-centre "topics" then, since they are not compatible with the current ones. We will need to keep the existing tables for a while for the old versions Ffx.
Flags: needinfo?(najiang)
Marina, we plan to log our new telemetry with a NEW table. Do we also need to change the filter ID?


The filter ID is `{filter: ONBOARDING_ID})` in onboarding now
https://searchfox.org/mozilla-central/source/browser/extensions/onboarding/OnboardingTelemetry.jsm#203

Do we need to change to something like `{filter: ONBOARDING_ID2})` for a new table?


(Besides change the ping-centre "topics" from "firefox-onboarding-event"/"firefox-onboarding-session" to "firefox-onboarding-event2"/"firefox-onboarding-session2")
Flags: needinfo?(msamuel)
Comment on attachment 8927141 [details]
Bug 1413830 - PART 2: properly handle sessions for onboarding telemetry;

https://reviewboard.mozilla.org/r/198386/#review206638

This depends on the bug 1418167. Without the bug 1418167 resolved, how to proceed?
Attachment #8927141 - Flags: review?(fliu)
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review206644

This depends on the bug 1418167. Without the bug 1418167 resolved, how to proceed?
Attachment #8927140 - Flags: review?(fliu)
The PRs look like the heads up so we will have an idea of what's next after bug 1418167, and make sure I didn't do the redundant work. These PRs are based on bug 1418167 so you can wait to review these PRs until bug 1418167 is resolved.

(Another reason is, with mozreview, I can't remove the review flag while update the PR :p)
(In reply to Fred Lin [:gasolin] from comment #60)
> The PRs look like the heads up so we will have an idea of what's next after
> bug 1418167, and make sure I didn't do the redundant work.
Yes, we all should save redundant works.
The 1st priority now is to make the schema definition, validation and the data-review done.
Then proceed to how to drop pings these detailed code stuff.

> These PRs are based on bug 1418167 so you can wait to review these PRs until bug 1418167
> is resolved.
> 
> (Another reason is, with mozreview, I can't remove the review flag while
> update the PR :p)
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review207204

Could we first get the data-review and the schema validation done then proceed to how to send out the pings. Without these 2 done, we cannot enable new pings. If any change, we might need to rethink data being sent
Attachment #8927140 - Flags: review?(fliu)
Comment on attachment 8927141 [details]
Bug 1413830 - PART 2: properly handle sessions for onboarding telemetry;

https://reviewboard.mozilla.org/r/198386/#review207206
Attachment #8927141 - Flags: review?(fliu)
Hi Chenxia, here's the updated data-review request (add the `width` column).
The data-review is within these patchs' scope, other column changes will be in a separate bug.


1) What questions will you answer with this data?

To analysis user interaction with onboarding tool for better browser retention.

2) Why does Mozilla need to answer these questions?  Are there benefits for users? Do we need this information to address product or business requirements?

We need this information to address product requirements. Data will be used for pursuing better browser retention rate.

3) What alternative methods did you consider to answer these questions? Why were they not sufficient?

We can use current overlay-nav-click and overlay-session event, but `overlay-nav-click` can't identify the user actually clicks the nav button or the user just see the tour because the tour showed to them by default.
overlay-session event can identify user open/close the overlay but can't identify how the user opens/close the overlay.
With UX request we also want to get the current screen width when user click the logo button so we can get some data if we need the responsive design.

4) Can current instrumentation answer these questions?

Part of but not very accurate

5) List all proposed measurements and indicate the category of data collection for each measurement, using the Firefox [data c](https://wiki.mozilla.org/Firefox/Data_Collection)[ollection ](https://wiki.mozilla.org/Firefox/Data_Collection)[categories](https://wiki.mozilla.org/Firefox/Data_Collection) on the found on the Mozilla wiki.   

<table>
  <tr>
    <td>Measurement Description</td>
    <td>Data Collection Category</td>
    <td>Tracking Bug #</td>
  </tr>
  <tr>
    <td>onboarding-logo-click triggered when user open the overlay via click the logo or the watermark</td>
    <td>Interaction data</td>
    <td>Bug 1413830</td>
  </tr>
  <tr>
    <td>overlay-current-tour event is sent when any tour is going to display on the overlay</td>
    <td>Interaction data</td>
    <td>Bug 1413830</td>
  </tr>
  <tr>
    <td>overlay-close-button-click triggered when user close the overlay via click the overlay close button</td>
    <td>Interaction data</td>
    <td>Bug 1413830</td>
  </tr>
  <tr>
    <td>overlay-close-outside-click triggered when user close the overlay via click outside of the tour content area</td>
    <td>Interaction data</td>
    <td>Bug 1413830</td>
  </tr>
  <tr>
    <td>notification-appear triggered when one notification appears</td>
    <td>Interaction data</td>
    <td>Bug 1413830</td>
  </tr>
  <tr>
    <td>rename event, tour_source, impression column to type, logo_state, notification_impression to help analyst better understand its purpose</td>
    <td>Interaction data</td>
    <td>Bug 1413830</td>
  </tr>
  <tr>
    <td>log separate root_session_id / parent_session_id so analyst can better identify the event comes from which overlay-session </td>
    <td>Interaction data</td>
    <td>Bug 1413830 PART 2 patch</td>
  </tr>
  <tr>
    <td>log screen width for onboarding-logo-click event so analyst can get data to decide if we need the responsive design</td>
    <td>Interaction data</td>
    <td>Bug 1413830 PART 1 patch</td>
  </tr>
</table>

Here's the proposed Telemetry events/column mapping
https://docs.google.com/spreadsheets/d/1BQzncwklWZvvkLtbLaYO2unMG3Tw9LQX-c6XXqmYTUo/edit#gid=1206558344

6) How long will this data be collected?  Choose one of the following:

* I want to permanently monitor this data. (Fred Lin)

7) What populations will you measure?

* Which release channels? All

* Which countries? All

* Which locales? All

* Any other filters?  Please describe in detail below.
No

8) Please provide a general description of how you will analyze this data.

Identify how user open/close the overlay and click the tour

9) Where do you intend to share the results of your analysis?

The data will be shared between PM & UX to drive the better decision
Since Chenxia is in PTO, francois could you help on the data review part?
Flags: needinfo?(liuche) → needinfo?(francois)
First of all, could you please copy these answers into a text file attached to the bug so that it's easy to see where the latest documentation about this probe is?

There's already two copies of the answers in the bug log and there would likely be a third copy.

Then you can request an r? on that file and I can leave comments in there.

(In reply to Fred Lin [:gasolin] from comment #74)
>     <td>rename event, tour_source, impression column to type, logo_state,
> notification_impression to help analyst better understand its purpose</td>
>     <td>Interaction data</td>
>     <td>Bug 1413830</td>

Can you expand the description on this one? It seems like you're logging several pieces of data whenever a "rename event" occurs. What exactly goes into a rename event ping?

What are the possible values of "tour_source". Is it a fixed list of strings?

I assume logo_state is a fixed list of strings?

>     <td>log separate root_session_id / parent_session_id so analyst can
> better identify the event comes from which overlay-session </td>
>     <td>Interaction data</td>
>     <td>Bug 1413830 PART 2 patch</td>

Where are these two IDs coming from? Are they unique for each user? Unique for each user for each browser session? Are they derived from existing identifiers?

>   </tr>
>   <tr>
>     <td>log screen width for onboarding-logo-click event so analyst can get
> data to decide if we need the responsive design</td>

Do you need the exact width or can you round it up to the nearest 50 or 100 pixels? If you don't need full precision, then collecting rounded values reduces the risk that this could be used to derive a unique user identifier.

By the way, feel free to use a different format than this HTML format since it's probably too limiting for your use case. These pings and the details of their contents probably would be easier to read/understand as a bullet list or as JSON.

> Here's the proposed Telemetry events/column mapping
> https://docs.google.com/spreadsheets/d/1BQzncwklWZvvkLtbLaYO2unMG3Tw9LQX-
> c6XXqmYTUo/edit#gid=1206558344

That Google doc is not publicly available, so if this data is needed as part of the data review, it should either be made public or copied into a different doc which can be made public.
Flags: needinfo?(francois)
(In reply to François Marier [:francois] from comment #86)
> Can you expand the description on this one?

This could be as simple as linking, from each new probe, to the relevant section in data_events.md if it contains everything needed. No need to duplicate the info.

Sorry, I responded to the NEEDINFO before reviewing PART 1 and 2.
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review207540

::: browser/extensions/onboarding/data_events.md:45
(Diff revision 20)
> +| `category` | [Required] Either ["", "overlay-interactions", "notification-interactions"] to identify which kind of the interaction | :one:
> +| `client_id` | [Required] An identifier generated by [ClientID](https://github.com/mozilla/gecko-dev/blob/master/toolkit/modules/ClientID.jsm) module to provide an identifier for this device. This data is automatically appended by `ping-centre` module | :one:
> +| `ip` | [Auto populated by Onyx] The IP address of the client. 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.| :two:
> +| `locale` | The browser chrome's language (e.g. en-US). | :two:
> +| `page` | [Required] One of ["about:newtab", "about:home"]| :one:
> +| `parent_session_id` | [Required] The unique identifier generated by `gUUIDGenerator` service to identify this event belong to which parent session. | :one:

What is `parent` referring to in this case?

::: browser/extensions/onboarding/data_events.md:46
(Diff revision 20)
> +| `client_id` | [Required] An identifier generated by [ClientID](https://github.com/mozilla/gecko-dev/blob/master/toolkit/modules/ClientID.jsm) module to provide an identifier for this device. This data is automatically appended by `ping-centre` module | :one:
> +| `ip` | [Auto populated by Onyx] The IP address of the client. 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.| :two:
> +| `locale` | The browser chrome's language (e.g. en-US). | :two:
> +| `page` | [Required] One of ["about:newtab", "about:home"]| :one:
> +| `parent_session_id` | [Required] The unique identifier generated by `gUUIDGenerator` service to identify this event belong to which parent session. | :one:
> +| `root_session_id` | [Required] The unique identifier generated by `gUUIDGenerator` service to identify this event belong to which root session. | :one:

Also, I'm not familiar with how `root` is used here.
Attachment #8927140 - Flags: review?(francois)
Comment on attachment 8927141 [details]
Bug 1413830 - PART 2: properly handle sessions for onboarding telemetry;

https://reviewboard.mozilla.org/r/198386/#review207544

::: browser/extensions/onboarding/data_events.md:51
(Diff revision 26)
>  | `page` | [Required] One of ["about:newtab", "about:home"]| :one:
>  | `parent_session_id` | [Required] The unique identifier generated by `gUUIDGenerator` service to identify this event belong to which parent session. | :one:
>  | `root_session_id` | [Required] The unique identifier generated by `gUUIDGenerator` service to identify this event belong to which root session. | :one:
>  | `session_begin` | [Required] Timestamp in (integer) milliseconds when onboarding/overlay/notification becoming visible. | :one:
>  | `session_end` | [Required] Timestamp in (integer) milliseconds when onboarding/overlay/notification losing focus. | :one:
> +sessions for onboarding telemetry;r=fischer,francois

Is this line intended? It looks like a copy/paste error.
Attachment #8927141 - Flags: review?(francois)
Blocks: 1419995
Blocks: 1419996
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review207540

> What is `parent` referring to in this case?

We will log different uuid when user create new tab, open the onboarding overlay, and open the onboarding notification. Events happens in overlay will have the `overlay session uuid` as its `parent_session_id`. Events happens in notification will have the `notification session uuid` as its `parent_session_id`.

> Also, I'm not familiar with how `root` is used here.

Every events will have the same `onboarding session uuid` as its `root_session_id` when they are interact in the same tab. Analyst could query with root_session_id to list all events interact within this session.
> First of all, could you please copy these answers into a text file attached to the bug so that it's easy to see where the latest documentation about this probe is?

Good idea! will attach data-review-request.md for data review.

> >     <td>log screen width for onboarding-logo-click event so analyst can get
> > data to decide if we need the responsive design</td>
> 
> Do you need the exact width or can you round it up to the nearest 50 or 100
> pixels? If you don't need full precision, then collecting rounded values
> reduces the risk that this could be used to derive a unique user identifier.

Bryant, instead of logging the exact screen width, does rounding to the nearest 50 or 100 pixels can still serve your purpose? If so please choose your preference for 50 or 100 pixels.
please check comment 91
Flags: needinfo?(bmao)
Attached file data-review-request — 
request data review via updated file.
Attachment #8931242 - Flags: review?(francois)
Summary: Data schema change for on boarding telemetry → Data schema change for onboarding telemetry
Comment on attachment 8931242 [details]
data-review-request

1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes: https://github.com/mozilla/gecko-dev/blob/master/browser/extensions/onboarding/data_events.md

2) Is there a control mechanism that allows the user to turn the data collection on and off? (Note, for data collection not needed for security purposes, Mozilla provides such a control mechanism) Provide details as to the control mechanism available.

Given this has already received a data review, I assume that the answer is yes and that the way to opt out is by disabling Telemetry.

Fred, can you confirm?

3) If the request is for permanent data collection, is there someone who will monitor the data over time?**

Fred Lin.

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?  **

Category 2

5) Is the data collection request for default-on or default-off?

Default on in all channels.

6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc.  See the appendix for more details)?

No, the new identifiers refer to existing ones. See comment 90.

7) Is the data collection covered by the existing Firefox privacy notice?

Yes.

8) Does there need to be a check-in in the future to determine whether to renew the data?

No.
Flags: needinfo?(gasolin)
Attachment #8931242 - Flags: review?(francois) → review+
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review207926

datareview+
Attachment #8927140 - Flags: review?(francois) → review+
Comment on attachment 8927141 [details]
Bug 1413830 - PART 2: properly handle sessions for onboarding telemetry;

https://reviewboard.mozilla.org/r/198386/#review207928

datareview+
Attachment #8927141 - Flags: review?(francois) → review+
> 1) Is there or will there be **documentation** that describes the schema for
> the ultimate data set available publicly, complete and accurate?
> 
> Yes:
> https://github.com/mozilla/gecko-dev/blob/master/browser/extensions/
> onboarding/data_events.md
> 
> 2) Is there a control mechanism that allows the user to turn the data
> collection on and off? (Note, for data collection not needed for security
> purposes, Mozilla provides such a control mechanism) Provide details as to
> the control mechanism available.
> 
> Given this has already received a data review, I assume that the answer is
> yes and that the way to opt out is by disabling Telemetry.
> 
> Fred, can you confirm?
> 

Yes, in first sentence of https://github.com/mozilla/gecko-dev/blob/master/browser/extensions/onboarding/data_events.md

"At any time, it is easy to turn off this data collection by opting out of Firefox telemetry."
https://support.mozilla.org/en-US/kb/share-telemetry-data-mozilla-help-improve-firefox
Flags: needinfo?(gasolin)
(In reply to Fred Lin [:gasolin] from comment #91)
> > First of all, could you please copy these answers into a text file attached to the bug so that it's easy to see where the latest documentation about this probe is?
> 
> Good idea! will attach data-review-request.md for data review.
> 
> > >     <td>log screen width for onboarding-logo-click event so analyst can get
> > > data to decide if we need the responsive design</td>
> > 
> > Do you need the exact width or can you round it up to the nearest 50 or 100
> > pixels? If you don't need full precision, then collecting rounded values
> > reduces the risk that this could be used to derive a unique user identifier.
> 
> Bryant, instead of logging the exact screen width, does rounding to the
> nearest 50 or 100 pixels can still serve your purpose? If so please choose
> your preference for 50 or 100 pixels.

50px sounds good to me. It should be enough for us to locate the width hotspot.
Flags: needinfo?(bmao)
Server-side table creation bug is tracked in https://github.com/mozilla/ping-centre/issues/127
updated PR based on granted bug 1418167
Comment on attachment 8931244 [details]
Bug 1413830 - PART 4: send bubble_state;

https://reviewboard.mozilla.org/r/202374/#review208626

datareview+
Attachment #8931244 - Flags: review?(francois) → review+
Comment on attachment 8931243 [details]
Bug 1413830 - PART 3: send notification_state;

https://reviewboard.mozilla.org/r/202372/#review208628

datareview+
Attachment #8931243 - Flags: review?(francois) → review+
Comment on attachment 8931244 [details]
Bug 1413830 - PART 4: send bubble_state;

https://reviewboard.mozilla.org/r/202374/#review208730

::: browser/extensions/onboarding/content/onboarding.js:794
(Diff revision 4)
>          this._overlayIcon.classList.remove("onboarding-watermark");
>          break;
>        case ICON_STATE_WATERMARK:
>          this._overlayIcon.classList.add("onboarding-watermark");
> +        telemetry({
> +          event: "watermark-class-added",

We don't need this internal ping. Simply to carry the bubble_state btw on sending out each ping is enough. This is error-prone and adds complexity.
Attachment #8931244 - Flags: review?(fliu)
Comment on attachment 8931243 [details]
Bug 1413830 - PART 3: send notification_state;

https://reviewboard.mozilla.org/r/202372/#review208732
Attachment #8931243 - Flags: review?(fliu)
Comment on attachment 8927141 [details]
Bug 1413830 - PART 2: properly handle sessions for onboarding telemetry;

https://reviewboard.mozilla.org/r/198386/#review208734
Attachment #8927141 - Flags: review?(fliu)
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review208726

In fact Part 1 depends on Part 2. Did Part 1 before Part is already a bit confusing... But now there are more Part 3 and Part 4, which seperately add the columns that are not newly created recently and should be included in the beginning. This is very error-prone.

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:552
(Diff revision 23)
> +          let logo_state = Services.prefs.getStringPref("browser.onboarding.state", "default");
> +          payload.logo_state = logo_state === "default" ? "logo" : logo_state;
> +        } else {
> +          payload.logo_state = "";
> +        }
> +        this._validatePayload(payload);

This definitely will throw because BASIC_EVENT_SCHEMA requires 'root_session_id` etc but we don't have
Attachment #8927140 - Flags: review?(fliu)
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review208766

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:552
(Diff revision 23)
> +          let logo_state = Services.prefs.getStringPref("browser.onboarding.state", "default");
> +          payload.logo_state = logo_state === "default" ? "logo" : logo_state;
> +        } else {
> +          payload.logo_state = "";
> +        }
> +        this._validatePayload(payload);

Yes, as comment it's in PART 2 patch. I'd merge these parts into single patch first, and see if we can split the patch in a meaningful way
Comment on attachment 8931244 [details]
Bug 1413830 - PART 4: send bubble_state;

https://reviewboard.mozilla.org/r/202374/#review208730

> We don't need this internal ping. Simply to carry the bubble_state btw on sending out each ping is enough. This is error-prone and adds complexity.

manage bubble state and notificaiton state in onboarding.js and send when required make sense, will merge these 2 patch in to a single one
Attachment #8927141 - Attachment is obsolete: true
Attachment #8931243 - Attachment is obsolete: true
Attachment #8931244 - Attachment is obsolete: true
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review209644

::: browser/extensions/onboarding/content/onboarding.js:465
(Diff revision 27)
>    _getTourIDList() {
>      let tours = Services.prefs.getStringPref(`browser.onboarding.${this._tourType}tour`, "");
>      return tours.split(",").filter(tourId => tourId !== "").map(tourId => tourId.trim());
>    }
>  
> -  _initNotification() {
> +  _initRender() {

Each time window rsizing and re-init UI, `_initUI` called, then `_initRender` called, then `_renderUIOnVisible` called, then a "onboarding-session-begin" telemetry sent. Is it that right? Remeberd "onboarding-session-begin" should be count one for one page load not during resizing.

::: browser/extensions/onboarding/content/onboarding.js:1068
(Diff revision 27)
>    }
>  
> +  /**
> +   * Render the UI when the tab is visible.
> +   */
> +  _renderUIOnVisible() {

This `_renderUIOnVisible` in fact is:
1. Not rendering UI (only notification)
2. Not doing onVisible. It will do right away when called disregarding visibility.
Move the codes here to `_initRender` should be enough

Another thing, if it wasn't required, put the telemetry after the main job done, this is for
1. We should have the main job done asap
2. In case anything wrong then we still have the main job executed.
Attachment #8927140 - Flags: review?(fliu)
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review209644

> Each time window rsizing and re-init UI, `_initUI` called, then `_initRender` called, then `_renderUIOnVisible` called, then a "onboarding-session-begin" telemetry sent. Is it that right? Remeberd "onboarding-session-begin" should be count one for one page load not during resizing.

fixed, only trigger the "onboarding-session-begin" telemetry when `_initUI` is called from `init` (not from resize)

> This `_renderUIOnVisible` in fact is:
> 1. Not rendering UI (only notification)
> 2. Not doing onVisible. It will do right away when called disregarding visibility.
> Move the codes here to `_initRender` should be enough
> 
> Another thing, if it wasn't required, put the telemetry after the main job done, this is for
> 1. We should have the main job done asap
> 2. In case anything wrong then we still have the main job executed.

Removed _renderUIOnVisible and _initRender, will call `_isPageShown(reason, callback)` instead

For the right order, the `onboarding-session-begin` should happen before any other telemetry event. 
Since `notification-session-begin` and `notification-appear` event will be called in `this.showNotification`, the `onboarding-session-begin` event should send before the `this.showNotification` call.
For debugging purpose, you can add some dump message in _validatePayload like

```
dump(`\n>> ${key} : ${payload[key]}...`);
results[key] = validators[key](payload[key]);
if (!results[key]) {
 failed = true;
}
dump(`${results[key]}`);
```

To check all events and columns comes in right order


And open browser toolbox to see if there's any exceptions
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review210004

::: browser/extensions/onboarding/content/onboarding.js:415
(Diff revision 28)
> -      event: "onboarding-session-begin",
> -      session_key: this._session_key,
> -    });
>    }
>  
> -  _resizeUI() {
> +  _resizeUI(reason) {

In fact don't need to add `reason`, which is another layer. Can just go something like:
``` 
// In `async init(contentWindow)`

let doc = this._window.document;
if (doc.hidden) {
  // When the preloaded-browser feature is on,
  // it would preload a hidden about:newtab in the background.
  // We don't want to show notification in that hidden state.
  let onVisible = () => {
    if (!doc.hidden) {
       doc.removeEventListener("visibilitychange", onVisible);
       this._resizeTimerId = this._window.requestIdleCallback(() => this._resizeUI());
       this._window.addEventListener("resize", this);
       telemetry({ event: "onboarding-session-begin", ... });
    }
  };
  doc.addEventListener("visibilitychange", onVisible);
} else {
  this._resizeTimerId = this._window.requestIdleCallback(() => this._resizeUI());
  this._window.addEventListener("resize", this);
  telemetry({ event: "onboarding-session-begin", ... });
}

```
Attachment #8927140 - Flags: review?(fliu)
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review210004

> In fact don't need to add `reason`, which is another layer. Can just go something like:
> ``` 
> // In `async init(contentWindow)`
> 
> let doc = this._window.document;
> if (doc.hidden) {
>   // When the preloaded-browser feature is on,
>   // it would preload a hidden about:newtab in the background.
>   // We don't want to show notification in that hidden state.
>   let onVisible = () => {
>     if (!doc.hidden) {
>        doc.removeEventListener("visibilitychange", onVisible);
>        this._resizeTimerId = this._window.requestIdleCallback(() => this._resizeUI());
>        this._window.addEventListener("resize", this);
>        telemetry({ event: "onboarding-session-begin", ... });
>     }
>   };
>   doc.addEventListener("visibilitychange", onVisible);
> } else {
>   this._resizeTimerId = this._window.requestIdleCallback(() => this._resizeUI());
>   this._window.addEventListener("resize", this);
>   telemetry({ event: "onboarding-session-begin", ... });
> }
> 
> ```

removed `reason` and moved onVisible detection into `init` function
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review210458

::: commit-message-ed94d:1
(Diff revision 30)
> +Bug 1413830 - Data schema change for onboarding telemetry;r=fischer,francois

Do you do all pings in this patch? It looks not. If not all, please put what pings are processed in the commit message so able to track the commit history

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:224
(Diff revision 30)
>    "onboarding-register-session": {topic: "internal"},
>    // track the start and end time of the onboarding session
>    "onboarding-session": {
>      topic: "firefox-onboarding-session2",
>      category: "onboarding-interactions",
> +    parent: "onboarding",

Call directly "onboarding-session", so explictit and easier connected

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:467
(Diff revision 30)
> +      switch (event) {
> +        case "onboarding-session-begin":
> +          this.state.sessions[session_key].onboarding_session_begin = Date.now();
> +          break;
> +        case "onboarding-session-end":
> +          this._sendPing("firefox-onboarding-session2", Object.assign({}, data, {

`_sendPing` inside the internal topic kinds of conflicts. Simply can go such as:
```
if (topic === "internal") {
   switch (event) {
     case "onboarding-session-begin":
      ... ...
      return;
      
     case case "onboarding-session-end":
       data = Object.assign({}, data, { ... }); 
       this.state.sessions[session_key].onboarding_session_end = Date.now();
       break;
       
     ... ...
   }
}
this._sendPing(topic, data);
```
And should set the session_end time here like "onboarding-session-begin"

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:468
(Diff revision 30)
> +        case "onboarding-session-begin":
> +          this.state.sessions[session_key].onboarding_session_begin = Date.now();
> +          break;
> +        case "onboarding-session-end":
> +          this._sendPing("firefox-onboarding-session2", Object.assign({}, data, {
> +            event: "onboarding-session"

set directly as `type` so aligned with inside `_sendPing`

::: browser/extensions/onboarding/content/onboarding.js:413
(Diff revision 30)
>      this.uiInitialized = false;
> +    let doc = this._window.document;
> +    if (doc.hidden) {
> +      // When the preloaded-browser feature is on,
> +      // it would preload a hidden about:newtab in the background.
> +      // We don't want to show notification in that hidden state.

Update this comment as well
Attachment #8927140 - Flags: review?(fliu)
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review210458

> Do you do all pings in this patch? It looks not. If not all, please put what pings are processed in the commit message so able to track the commit history

This patch covers all new events except onboarding-noshow-smallscreen and overlay-disapear-resize events will be implemented in bug 1417769

> Call directly "onboarding-session", so explictit and easier connected

updated

> `_sendPing` inside the internal topic kinds of conflicts. Simply can go such as:
> ```
> if (topic === "internal") {
>    switch (event) {
>      case "onboarding-session-begin":
>       ... ...
>       return;
>       
>      case case "onboarding-session-end":
>        data = Object.assign({}, data, { ... }); 
>        this.state.sessions[session_key].onboarding_session_end = Date.now();
>        break;
>        
>      ... ...
>    }
> }
> this._sendPing(topic, data);
> ```
> And should set the session_end time here like "onboarding-session-begin"

updated

> set directly as `type` so aligned with inside `_sendPing`

updated. Note change from `event` to `type` makes all old pings not work. We'll remove old pings soon though.

> Update this comment as well

updated
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review210908

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:448
(Diff revision 32)
>      }
>    },
>  
> +  processPings(data) {
> +    let { type, session_key } = data;
> +    let topic = EVENT_WHITELIST[type] && EVENT_WHITELIST[type].topic;



::: browser/extensions/onboarding/OnboardingTelemetry.jsm:448
(Diff revision 32)
>      }
>    },
>  
> +  processPings(data) {
> +    let { type, session_key } = data;
> +    let topic = EVENT_WHITELIST[type] && EVENT_WHITELIST[type].topic;

Remove here

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:497
(Diff revision 32)
> +      }
> +      topic = EVENT_WHITELIST[data.type] && EVENT_WHITELIST[data.type].topic;
> +      if (!topic) {
> +        throw new Error(`ping-centre doesn't know ${type} after processPings, only knows ${Object.keys(EVENT_WHITELIST)}`);
> +      }
> +      this._sendPing(topic, data);

Move this down

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:499
(Diff revision 32)
> +      if (!topic) {
> +        throw new Error(`ping-centre doesn't know ${type} after processPings, only knows ${Object.keys(EVENT_WHITELIST)}`);
> +      }
> +      this._sendPing(topic, data);
> +    } else {
> +      this._sendPing(topic, data);

Do 
```
      topic = EVENT_WHITELIST[data.type] && EVENT_WHITELIST[data.type].topic;
      if (!topic) {
        throw new Error(`ping-centre doesn't know ${type} after processPings, only knows ${Object.keys(EVENT_WHITELIST)}`);
      }
      this._sendPing(topic, data);
```
here

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:542
(Diff revision 32)
> +    let payload;
> +    let session_begin;
> +    let session_end;
> +    let session_id;
> +    if (!onboarding_session_id) {
> +      throw new Error(`should fire onboarding-register-session event before ${type}`);

Put this throw in `processPings` where:
```
case "onboarding-session-begin":
  this.state.sessions[session_key].onboarding_session_begin = Date.now();
```

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:545
(Diff revision 32)
> +    let session_id;
> +    if (!onboarding_session_id) {
> +      throw new Error(`should fire onboarding-register-session event before ${type}`);
> +    }
> +    let root_session_id = onboarding_session_id;
> +    switch (parent) {

Thinking extract this `parent_session_id` switch case into another function to reduce some size of `_sendPing` but no strong opinion, up to you.
However, please update the switch to like A or B version:
```
// Version A
switch (parent) {
  case "onboarding-session":
    parent_session_id = onboarding_session_id;
    break;
    
    ... ...
}
if (!parent_session_id) {
  throw ....;
}
```

OR

```
// Version B
let session = this.state.sessions[session_key];
parent_session_id = session[`${parent}_id`];
if (!parent_session_id) {
  throw ....;
}
```

so less `throw` statements and take care the unknown parent case as well

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:585
(Diff revision 32)
> +            session_begin = overlay_session_begin;
> +            session_end = overlay_session_end;
> +            break;
> +          case "notification-session":
> +            if (!notification_session_begin || !notification_session_end) {
> +              throw new Error(`should fire notification-session-begin and notification_session_end event before ${type}`);

Remove all the `throw` statements inside the `switch(type)` and do after the switch like:
```
if (!session_id || !session_begin || !session_end) {
  throw ...;
}
```
Attachment #8927140 - Flags: review?(fliu)
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review210908

> Remove here

remove if (topic === "internal") check and add 

```
if (topic === "internal") {
  throw new Error(`internal ping ${type} should be processed within processPings`);
}
```

before call `_sendPing`

> Do 
> ```
>       topic = EVENT_WHITELIST[data.type] && EVENT_WHITELIST[data.type].topic;
>       if (!topic) {
>         throw new Error(`ping-centre doesn't know ${type} after processPings, only knows ${Object.keys(EVENT_WHITELIST)}`);
>       }
>       this._sendPing(topic, data);
> ```
> here

updated

> Put this throw in `processPings` where:
> ```
> case "onboarding-session-begin":
>   this.state.sessions[session_key].onboarding_session_begin = Date.now();
> ```

updated

> Thinking extract this `parent_session_id` switch case into another function to reduce some size of `_sendPing` but no strong opinion, up to you.
> However, please update the switch to like A or B version:
> ```
> // Version A
> switch (parent) {
>   case "onboarding-session":
>     parent_session_id = onboarding_session_id;
>     break;
>     
>     ... ...
> }
> if (!parent_session_id) {
>   throw ....;
> }
> ```
> 
> OR
> 
> ```
> // Version B
> let session = this.state.sessions[session_key];
> parent_session_id = session[`${parent}_id`];
> if (!parent_session_id) {
>   throw ....;
> }
> ```
> 
> so less `throw` statements and take care the unknown parent case as well

update to Version A, I'd rather not extract to another function since these code is only used here

> Remove all the `throw` statements inside the `switch(type)` and do after the switch like:
> ```
> if (!session_id || !session_begin || !session_end) {
>   throw ...;
> }
> ```

updated
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review211298

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:496
(Diff revision 34)
> +    let topic = EVENT_WHITELIST[data.type] && EVENT_WHITELIST[data.type].topic;
> +    if (!topic) {
> +      throw new Error(`ping-centre doesn't know ${type} after processPings, only knows ${Object.keys(EVENT_WHITELIST)}`);
> +    }
> +    if (topic === "internal") {
> +      throw new Error(`internal ping ${type} should be processed within processPings`);

We are inside the `processPings` at this moment so 
move this throw into the `_sendPing`

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:554
(Diff revision 34)
> +      case "notification-session":
> +        parent_session_id = notification_session_id;
> +        break;
> +    }
> +    if (!parent_session_id) {
> +      throw new Error(`should fire ${type}-begin event before ${type}`);

`type` may not be the session event type, could be "notification-cta-click" when arriving here so maybe: "Unable to find the ${parent} parent session for the event ${type}"

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:578
(Diff revision 34)
> +            session_begin = notification_session_begin;
> +            session_end = notification_session_end;
> +            break;
> +        }
> +        if (!session_id || !session_begin || !session_end) {
> +          throw new Error(`should fire ${type}-begin and ${type}_end event before ${type}`);

${type}-end

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:595
(Diff revision 34)
> +          tour_type,
> +          type,
> +        };
> +        this._validatePayload(payload);
> +        this.sessionProbe && this.sessionProbe.sendPing(payload,
> +          {filter: ONBOARDING_ID});

Do we have the `filter` confirmed?

::: browser/extensions/onboarding/content/onboarding.js:438
(Diff revision 34)
>        session_key: this._session_key,
>      });
>    }
>  
>    _resizeUI() {
> -    let width = this._window.document.body.getBoundingClientRect().width;
> +    this.width = this._window.document.body.getBoundingClientRect().width;

this._docWidth or this._windowWidth or some name showing this is about the total window width and aligned with this._roundedDocWidth

::: browser/extensions/onboarding/content/onboarding.js:448
(Diff revision 34)
>      }
>  
>      this._initUI();
> -    if (this._isFirstSession && width >= SPEECH_BUBBLE_MIN_WIDTH_PX) {
> +    if (this._isFirstSession && this.width >= SPEECH_BUBBLE_MIN_WIDTH_PX) {
>        this._overlayIcon.classList.add("onboarding-speech-bubble");
> +      this._bubbleState = "bubble";

What if for any reason we entered the below `      this._overlayIcon.classList.remove("onboarding-speech-bubble")`? Make `_bubbleState` as a getter
```
get _bubbleState() {
  return "bubble" if "onboarding-speech-bubble" else if ... else ...
}
```

::: browser/extensions/onboarding/content/onboarding.js:563
(Diff revision 34)
> +  /**
> +   * Return current screen width and round it up to the nearest 50 pixels.
> +   * Collecting rounded values reduces the risk that this could be used to
> +   * derive a unique user identifier
> +   */
> +  get _roundWidth() {

_roundedDocWidth or _roundedWindowWidth (aligned with this._docWidth)
Attachment #8927140 - Flags: review?(fliu)
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review211298

> We are inside the `processPings` at this moment so 
> move this throw into the `_sendPing`

moved to `_sendPing`

> `type` may not be the session event type, could be "notification-cta-click" when arriving here so maybe: "Unable to find the ${parent} parent session for the event ${type}"

updated

> ${type}-end

updated

> Do we have the `filter` confirmed?

Was asked in comment 51 but no response yet, will ping again.

> this._docWidth or this._windowWidth or some name showing this is about the total window width and aligned with this._roundedDocWidth

update to `_windowWidth` & `_roundWindowWidth`

> What if for any reason we entered the below `      this._overlayIcon.classList.remove("onboarding-speech-bubble")`? Make `_bubbleState` as a getter
> ```
> get _bubbleState() {
>   return "bubble" if "onboarding-speech-bubble" else if ... else ...
> }
> ```

updated
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review211298

> Was asked in comment 51 but no response yet, will ping again.

OK, based on Marina's feedbacks on the slack. We need to have this `filter` as a substring of the shield experiment ID. Looked into PingCentre.jsm, it filters out the shield ID based on the `filter`[1] and sends it back to the Ping Centre server so that we will be  able to filter out the data collected under some shield experiment. Please checked the shield Id we used and looks like we have to stick with that `filter` in the future.


[1] https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/browser/modules/PingCentre.jsm#96
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review211712

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:535
(Diff revision 36)
> +    } = EVENT_WHITELIST[type];
> +    let parent_session_id;
> +    let payload;
> +    let session_begin;
> +    let session_end;
> +    let session_id;



::: browser/extensions/onboarding/OnboardingTelemetry.jsm:539
(Diff revision 36)
> +    let session_end;
> +    let session_id;
> +    let root_session_id = onboarding_session_id;
> +
> +    if (topic === "internal") {
> +      throw new Error(`internal ping ${type} should be processed within processPings`);

Could we put this in the top?

::: browser/extensions/onboarding/content/onboarding.js:808
(Diff revision 36)
>  
>      this._overlayIcon.dispatchEvent(new this._window.CustomEvent("Agent:Destroy"));
>  
>      this._clearPrefObserver();
>      this._overlayIcon.remove();
> +    if (this._overlay && this._overlay.classList.contains("onboarding-opened")) {

Move the check into `hideOverlay`

::: browser/extensions/onboarding/content/onboarding.js:812
(Diff revision 36)
>      this._overlayIcon.remove();
> +    if (this._overlay && this._overlay.classList.contains("onboarding-opened")) {
> +      // send overlay-session telemetry
> +      this.hideOverlay();
> +    }
>      this._overlay.remove();

Now need 
``` 
this.hideOverlay();
this._overlay && this._overlay.remove();
```
OR
```
if (this._overlay) {
  this.hideOverlay();
  this._overlay.remove();
}
```
because not starting UI right away anymore

::: browser/extensions/onboarding/content/onboarding.js:841
(Diff revision 36)
>        // Lazy loading until first toggle.
>        this._loadTours(this._tours);
>      }
>  
>      this.hideNotification();
>      this.toggleModal(this._overlay.classList.toggle("onboarding-opened"));

This will hide a opened overlay. Call `add` with checking to improve it and make sure the ping sent correctly btw.

::: browser/extensions/onboarding/content/onboarding.js:849
(Diff revision 36)
>        session_key: this._session_key
>      });
>    }
>  
>    hideOverlay() {
>      this.toggleModal(this._overlay.classList.toggle("onboarding-opened"));

Currently this will show the hidden overlay please improve it btw. Call `remove` with the check
Attachment #8927140 - Flags: review?(fliu)
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review211712

> Could we put this in the top?

updated

> Move the check into `hideOverlay`

moved

> Now need 
> ``` 
> this.hideOverlay();
> this._overlay && this._overlay.remove();
> ```
> OR
> ```
> if (this._overlay) {
>   this.hideOverlay();
>   this._overlay.remove();
> }
> ```
> because not starting UI right away anymore

updated

> This will hide a opened overlay. Call `add` with checking to improve it and make sure the ping sent correctly btw.

updated

> Currently this will show the hidden overlay please improve it btw. Call `remove` with the check

updated
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review212222

::: browser/extensions/onboarding/content/onboarding.js:546
(Diff revision 37)
> +
> +  /**
> +   * Return current logo state as "logo" or "watermark".
> +   */
> +  get _logoState() {
> +    let state = Services.prefs.getStringPref("browser.onboarding.state", "default");

Follow `_onIconStateChange` to decide the state from the `classList`. The cost is cheaper.

::: browser/extensions/onboarding/content/onboarding.js:576
(Diff revision 37)
> +   * Return current screen width and round it up to the nearest 50 pixels.
> +   * Collecting rounded values reduces the risk that this could be used to
> +   * derive a unique user identifier
> +   */
> +  get _roundWindowWidth() {
> +    return Math.round(this._windowWidth / 50) * 50;

nit: `_roundedWindowWidth`. `_roundWindowWidth` means like the width of a round(circular) window.

::: browser/extensions/onboarding/content/onboarding.js:622
(Diff revision 37)
> +          width: this._roundWindowWidth,
> +        });
>          this.hideOverlay();
>          break;
>        case "onboarding-notification-close-btn":
> -        let tour_id = this._notificationBar.dataset.targetTourId;
> +        let current_tour_id = this._notificationBar.dataset.targetTourId;

nit: use camal case for all the variables just like all the others. The last time didn't correct this. This time fix this btw. Only a few words typing, no need to save...

::: browser/extensions/onboarding/content/onboarding.js:626
(Diff revision 37)
>        case "onboarding-notification-close-btn":
> -        let tour_id = this._notificationBar.dataset.targetTourId;
> +        let current_tour_id = this._notificationBar.dataset.targetTourId;
>          this.hideNotification();
> -        this._removeTourFromNotificationQueue(tour_id);
> +        this._removeTourFromNotificationQueue(current_tour_id);
>          telemetry({
> -          event: "notification-close-button-click",
> +          type: "notification-close-button-click",

Now the timing matters here. "notification-close-button-click" should exist inside "notification-session" so it should go before `hideNotification`. Maybe a comment is required for this (Not really like this but haven't got a good idea)

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

The timing matters here. Move "notification-cta-click" before `showOverlay`;

::: browser/extensions/onboarding/content/onboarding.js:840
(Diff revision 37)
>      if (this._tourItems.length == 0) {
>        // Lazy loading until first toggle.
>        this._loadTours(this._tours);
>      }
>  
>      this.hideNotification();

Move `hideNotification` into `if (this._overlay && !this._overlay.classList.contains("onboarding-opened"))`

::: browser/extensions/onboarding/content/onboarding.js:1028
(Diff revision 37)
>      let muteDuration = Services.prefs.getIntPref("browser.onboarding.notification.mute-duration-on-first-session-ms");
>      return Date.now() - lastTourChangeTime <= muteDuration;
>    }
>  
>    _isTimeForNextTourNotification(lastTourChangeTime) {
> -    let promptCount = Services.prefs.getIntPref("browser.onboarding.notification.prompt-count", 0);
> +    let promptCount = Services.prefs.getIntPref(PROMPT_COUNT_PREF, 0);

Reuse `this._notificationPromptCount`

::: browser/extensions/onboarding/content/onboarding.js:1084
(Diff revision 37)
>      return queue ? queue.split(",") : [];
>    }
>  
>    showNotification() {
>      if (Services.prefs.getBoolPref("browser.onboarding.notification.finished", false)) {
> +      this._notificationState = "finished";

Quite easy to miss the notification state by setting it through the code flow, many places to consider. Please make `_notificationState` a getter:
```
get _notificationState() {
  return "show" if ... else if ... else ...;
}
```

::: browser/extensions/onboarding/content/onboarding.js:1130
(Diff revision 37)
>          {
>            name: "browser.onboarding.state",
>            value: ICON_STATE_WATERMARK
>          }
>        ]);
>        return;

if `this._muteNotificationOnFirstSession(lastTime)` was false and `Services.prefs.getBoolPref("browser.onboarding.notification.finished", false)` was false on the top, but we arrived here, then `_notificationState` would be wrong.

::: browser/extensions/onboarding/content/onboarding.js:1166
(Diff revision 37)
>        params.push({
>          name: "browser.onboarding.notification.tour-ids-queue",
>          value: queue.join(",")
>        });
>      } else {
> -      let promptCount = Services.prefs.getIntPref(PROMPT_COUNT_PREF, 0);
> +      promptCount = Services.prefs.getIntPref(PROMPT_COUNT_PREF, 0) + 1;

Reuse `this._notificationPromptCount` since it is created

::: browser/extensions/onboarding/data_events.md:95
(Diff revision 37)
>  |-----|-------------|:-----:|
>  | `addon_version` | [Required] The version of the Onboarding addon. | :one:
> +| `bubble_state` | [Optional] | One of ["bubble", "dot", "hide"] indicates the current visual state of the speach bubble (content dialog besides the onboarding logo). | :one:
>  | `category` | [Required] Either ("overlay-interactions", "notification-interactions") to identify which kind of the interaction | :one:
>  | `client_id` | [Required] An identifier generated by [ClientID](https://github.com/mozilla/gecko-dev/blob/master/toolkit/modules/ClientID.jsm) module to provide an identifier for this device. This data is automatically appended by `ping-centre` module | :one:
> -| `event` | [Required] The type of event. allowed event strings are defined in the below section | :one:
> +| `current_tour_id` | [Optional] id of the current tour. The number of open from notification can be retrieved via 'notification-cta-click event'. We put "" when this field is not relevant to this event | :one:

"notification-cta-click" comments have to do with "current_tour_id"?

::: browser/extensions/onboarding/data_events.md:98
(Diff revision 37)
>  | `client_id` | [Required] An identifier generated by [ClientID](https://github.com/mozilla/gecko-dev/blob/master/toolkit/modules/ClientID.jsm) module to provide an identifier for this device. This data is automatically appended by `ping-centre` module | :one:
> -| `event` | [Required] The type of event. allowed event strings are defined in the below section | :one:
> +| `current_tour_id` | [Optional] id of the current tour. The number of open from notification can be retrieved via 'notification-cta-click event'. We put "" when this field is not relevant to this event | :one:
> -| `impression` | [Optional] An integer to record how many times the current notification tour is shown to the user. Each Notification tour can show not more than 8 times. We put `-1` when this field is not relevant to this event | :one:
>  | `ip` | [Auto populated by Onyx] The IP address of the client. 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.| :two:
>  | `locale` | The browser chrome's language (e.g. en-US). | :two:
> +| `logo_state` | [Optional] One of ["logo", "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'. | :one:

"notification-cta-click" comments have to do with "logo_state"?

::: browser/extensions/onboarding/data_events.md:104
(Diff revision 37)
> +| `notification_impression` | [Optional] An integer to record how many times the current notification tour is shown to the user. Each Notification tour can show not more than 8 times. We put `-1` when this field is not relevant to this event | :one:
> +| `notification_state` | [Optional] One of ["show", "hide", "finished"] indicates the current notification bar state. | :one:
>  | `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 the overlay is opened/when notification is shown. | :one:
> +| `parent_session_id` | [Required] The unique identifier generated by `gUUIDGenerator` service to identify this event belongs to which parent session. Events happen upon overlay will have the `overlay session uuid` as its `parent_session_id`. Events happen upon notification will have the `notification session uuid` as its `parent_session_id`. | :one:
> +| `root_session_id` | [Required] The unique identifier generated by `gUUIDGenerator` service to identify this event belongs to which root session. Every event will have the same `onboarding session uuid` as its `root_session_id` when interact in the same tab. | :one:
> +| `target_tour_id` | [Optional] id of the target switched tour. The number of open from notification can be retrieved via 'notification-cta-click event'. We put "" when this field is not relevant to this event | :one:

"notification-cta-click" comments have to do with here?

::: browser/extensions/onboarding/data_events.md:151
(Diff revision 37)
>  
>  | EVENT | DESCRIPTION |
>  |-----------|---------------------|
> +| `notification-appear` | event is sent when a notification appears. |
> +| `notification-close-button-click` | event is sent when a user clicks close notification button. |
> +| `notification-cta-click` | event is sent when a user clicks the notification's Call-To-Action button. |

You probably will want to add the comments about "The number of open from..." here
Attachment #8927140 - Flags: review?(fliu)
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review212222

> Follow `_onIconStateChange` to decide the state from the `classList`. The cost is cheaper.

now detect via `this._overlayIcon.classList.contains("onboarding-watermark")`

> nit: `_roundedWindowWidth`. `_roundWindowWidth` means like the width of a round(circular) window.

rename to `_windowWidthRounded`

> nit: use camal case for all the variables just like all the others. The last time didn't correct this. This time fix this btw. Only a few words typing, no need to save...

updated to `currentTourId`

> Now the timing matters here. "notification-close-button-click" should exist inside "notification-session" so it should go before `hideNotification`. Maybe a comment is required for this (Not really like this but haven't got a good idea)

moved ahead of `this.hideNotification();` and add comment `should trigger before notification-session event is sent`

> The timing matters here. Move "notification-cta-click" before `showOverlay`;

updated

> Move `hideNotification` into `if (this._overlay && !this._overlay.classList.contains("onboarding-opened"))`

updated

> Reuse `this._notificationPromptCount`

updated

> Quite easy to miss the notification state by setting it through the code flow, many places to consider. Please make `_notificationState` a getter:
> ```
> get _notificationState() {
>   return "show" if ... else if ... else ...;
> }
> ```

updated. 

I hesitated if we need to move `_notificationState` to getter, since we will need to call `_getLastTourChangeTime`, `NOTIFICATION_FINISHED_PREF`, and `_muteNotificationOnFirstSession` each time when we want to send the notification state.

> if `this._muteNotificationOnFirstSession(lastTime)` was false and `Services.prefs.getBoolPref("browser.onboarding.notification.finished", false)` was false on the top, but we arrived here, then `_notificationState` would be wrong.

moved to _notificatoinState getter, don't have to check the queue.length since the queue length will reflect to the perf finished state.

> Reuse `this._notificationPromptCount` since it is created

updated

> "notification-cta-click" comments have to do with "current_tour_id"?

It seems not relevant, removed

> "notification-cta-click" comments have to do with "logo_state"?

It seems not relevant, removed

> "notification-cta-click" comments have to do with here?

not relevant, removed

> You probably will want to add the comments about "The number of open from..." here

we don't need that anymore since we added `onboarding-logo-click` event
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review211298

> OK, based on Marina's feedbacks on the slack. We need to have this `filter` as a substring of the shield experiment ID. Looked into PingCentre.jsm, it filters out the shield ID based on the `filter`[1] and sends it back to the Ping Centre server so that we will be  able to filter out the data collected under some shield experiment. Please checked the shield Id we used and looks like we have to stick with that `filter` in the future.
> 
> 
> [1] https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/browser/modules/PingCentre.jsm#96

shield experiment ID is defined per experiment, we  can stick with current `onboarding` ID. Just need to make sure all related experient contains "onboarding" substring in its  shield experiment ID
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review211298

> shield experiment ID is defined per experiment, we  can stick with current `onboarding` ID. Just need to make sure all related experient contains "onboarding" substring in its  shield experiment ID

I meant this as well. Yes please make sure the notification shield experiment we are doing is aligned with the `filter`
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review212222

> we don't need that anymore since we added `onboarding-logo-click` event

The comment was "The number of open from notification can be retrieved via 'notification-cta-click event'.", which dosen't conflict with `onboarding-logo-click`. Anyway, ok if you didn't want it.
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review213206

::: browser/extensions/onboarding/content/onboarding.js:540
(Diff revision 41)
>  
> +  /*
> +   * Return currently showing tour navigation item
> +   */
> +  get _activeTourId() {
> +    return this._tourItems.find(item => item.classList.contains("onboarding-active")).id;

What if `this._tourItems.find` found nothing or `_tourItems` is null? Should we return "" or null?

::: browser/extensions/onboarding/content/onboarding.js:572
(Diff revision 41)
> +   * Return current notification state as "show", "hide" or "finished".
> +   */
> +  get _notificationState() {
> +    let state;
> +    let lastTime = this._getLastTourChangeTime();
> +    if (Services.prefs.getBoolPref(NOTIFICATION_FINISHED_PREF, false)) {

Should cache the NOTIFICATION_FINISHED_PREF result like `_isFirstSession`. Do:
```
//Please name _theState a meaningful name
if (this._theState == "finished") {
  // Cache this should be fine it will get cleared on every page reload.
  // And only returning on "finished" is essential.
  // Assume in the "hide", it only means the notification dosen't appear but may be finished later. The "show" case is a bit differernt but similar. Only test against "finished" makes things simpler.
  return this._theState;
}

if (this._notification) {
  this._theState = "show";
} else if (Services.prefs.getBoolPref(NOTIFICATION_FINISHED_PREF, false)) {
  this._theState = "finished";
} else {
  // Don't have to check `_getLastTourChangeTime`.
  // If no the notification bar, then we know it is in the hidden state.
  this._theState = "hide";
}

return this._theState;
```

::: browser/extensions/onboarding/content/onboarding.js:611
(Diff revision 41)
>      switch (id) {
>        case "onboarding-overlay-button":
>          this.showOverlay();
>          this.gotoPage(this._firstUncompleteTour.id);
> +        telemetry({
> +          type: "onboarding-logo-click",

timing matters

::: browser/extensions/onboarding/content/onboarding.js:1103
(Diff revision 41)
>    showNotification() {
> -    if (Services.prefs.getBoolPref("browser.onboarding.notification.finished", false)) {
> -      return;
> -    }
> -
>      let lastTime = this._getLastTourChangeTime();

This is a bit nit however please move `let lastTime = this._getLastTourChangeTime();` back to its original code flow because:
1. it is involved in getting a pref so nice to avoid that when the notification is finished
2. the original way works fine and avoid the extra pref access so no need to update.

::: browser/extensions/onboarding/content/onboarding.js:1104
(Diff revision 41)
> -    if (Services.prefs.getBoolPref("browser.onboarding.notification.finished", false)) {
> -      return;
> -    }
> -
>      let lastTime = this._getLastTourChangeTime();
> -    if (this._muteNotificationOnFirstSession(lastTime)) {
> +    if (Services.prefs.getBoolPref(NOTIFICATION_FINISHED_PREF, false) ||

reuse `this._notificationState == "finished"
Attachment #8927140 - Flags: review?(fliu)
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review213206

> Should cache the NOTIFICATION_FINISHED_PREF result like `_isFirstSession`. Do:
> ```
> //Please name _theState a meaningful name
> if (this._theState == "finished") {
>   // Cache this should be fine it will get cleared on every page reload.
>   // And only returning on "finished" is essential.
>   // Assume in the "hide", it only means the notification dosen't appear but may be finished later. The "show" case is a bit differernt but similar. Only test against "finished" makes things simpler.
>   return this._theState;
> }
> 
> if (this._notification) {
>   this._theState = "show";
> } else if (Services.prefs.getBoolPref(NOTIFICATION_FINISHED_PREF, false)) {
>   this._theState = "finished";
> } else {
>   // Don't have to check `_getLastTourChangeTime`.
>   // If no the notification bar, then we know it is in the hidden state.
>   this._theState = "hide";
> }
> 
> return this._theState;
> ```

The order should be
```
if (Services.prefs.getBoolPref(NOTIFICATION_FINISHED_PREF, false)) {

} else if (this._notification) {

} else {
  
}
```
Mis-placed in the previous fast typing.
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review213264

::: browser/extensions/onboarding/content/onboarding.js:1103
(Diff revision 41)
>    showNotification() {
> -    if (Services.prefs.getBoolPref("browser.onboarding.notification.finished", false)) {
> -      return;
> -    }
> -
>      let lastTime = this._getLastTourChangeTime();

restored
Flags: needinfo?(msamuel)
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review213206

> What if `this._tourItems.find` found nothing or `_tourItems` is null? Should we return "" or null?

Still what if `_tourItems` is null? Handle that case.
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review215718

::: browser/extensions/onboarding/content/onboarding.js:575
(Diff revision 43)
> +  get _notificationState() {
> +    if (this._notificationCachedState === "finished") {
> +      return this._notificationCachedState;
> +    }
> +
> +    // we know it is in the hidden state if there's no notification bar

Move this comment near `this._notificationCachedState = "hide";` because it is about that.
Attachment #8927140 - Flags: review?(fliu)
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review215718

> Move this comment near `this._notificationCachedState = "hide";` because it is about that.

moved the comment near `this._notificationCachedState = "hide"; `
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review215744

::: browser/extensions/onboarding/content/onboarding.js:541
(Diff revision 43)
> +  /*
> +   * Return currently showing tour navigation item
> +   */
> +  get _activeTourId() {
> +    let tourItem = this._tourItems.find(item => item.classList.contains("onboarding-active"));
> +    return tourItem ? tourItem.id : "";

the current patch already return "" when not found id
> 
> > What if `this._tourItems.find` found nothing or `_tourItems` is null? Should we return "" or null?
> 
> Still what if `_tourItems` is null? Handle that case.

this._tourItems should not be null or we'll get error early at showOverlay.

https://searchfox.org/mozilla-central/source/browser/extensions/onboarding/content/onboarding.js#745

all _activeTourId are called after showOverlay, so we don't need to deal with `_tourItems is null` case.
(In reply to Fred Lin [:gasolin] from comment #171)
> Comment on attachment 8927140 [details]
> Bug 1413830 - Data schema change for onboarding telemetry;
> 
> https://reviewboard.mozilla.org/r/198384/#review215744
> 
> ::: browser/extensions/onboarding/content/onboarding.js:541 (Diff revision 43)
> > +  /*
> > +   * Return currently showing tour navigation item
> > +   */
> > +  get _activeTourId() {
> > +    let tourItem = this._tourItems.find(item => item.classList.contains("onboarding-active"));
> > +    return tourItem ? tourItem.id : "";
> 
> the current patch already return "" when not found id
Yeah i knew this because I asked to handle this in the Comment 162

(In reply to Fred Lin [:gasolin] from comment #172)
> > > What if `this._tourItems.find` found nothing or `_tourItems` is null? Should we return "" or null?
> > 
> > Still what if `_tourItems` is null? Handle that case.
> 
> this._tourItems should not be null or we'll get error early at showOverlay.
> 
> https://searchfox.org/mozilla-central/source/browser/extensions/onboarding/content/onboarding.js#745
> 
> all _activeTourId are called after showOverlay, so we don't need to deal with `_tourItems is null` case.

`_tourItems` is set as array after `_initUI` is executed and gets removed in `destroy` when resizing window. That means if accessed `_activeTourId` outside that time frame, we threw because `tourItems` was null. Maybe now no code is doing that but still we should take care of that for the future. And since we are doing the lazy init stuff, we should take of that. Please improve this.
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review215758
Attachment #8927140 - Flags: review?(fliu)
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review213206

> Still what if `_tourItems` is null? Handle that case.

reuse markTourCompletionState's check to return "" if !this._tourItems || this._tourItems.length === 0

> The order should be
> ```
> if (Services.prefs.getBoolPref(NOTIFICATION_FINISHED_PREF, false)) {
> 
> } else if (this._notification) {
> 
> } else {
>   
> }
> ```
> Mis-placed in the previous fast typing.

nice to detect via this._notification, updated

> timing matters

updated

> reuse `this._notificationState == "finished"

updated
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review216122

::: browser/extensions/onboarding/content/onboarding.js:542
(Diff revision 45)
> +  /*
> +   * Return currently showing tour navigation item
> +   */
> +  get _activeTourId() {
> +    // We are doing lazy load so there might be no items.
> +    if (!this._tourItems || this._tourItems.length === 0) {

Below 
```
let tourItem = this._tourItems.find(....);
return tourItem ? tourItem.id : "";
```
codes has covered `this._tourItems.length === 0` case so only `!this._tourItems` enough
Attachment #8927140 - Flags: review?(fliu)
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review216122

> Below 
> ```
> let tourItem = this._tourItems.find(....);
> return tourItem ? tourItem.id : "";
> ```
> codes has covered `this._tourItems.length === 0` case so only `!this._tourItems` enough

I think early return takes no harm, but its fine to let the later case cover the  `this._tourItems.length === 0` case
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

https://reviewboard.mozilla.org/r/198384/#review216620
Attachment #8927140 - Flags: review?(fliu) → review+
Pushed by flin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8ba13ef1fec4
Data schema change for onboarding telemetry;r=Fischer,francois
Thanks for catching the issue. 
Changed `beforeunload` back to `unload` will solve the test failure, but the close sessions events will not be send. So we can't do this way.

I noticed the failure case shows [JavaScript Error: "Ping failure with error: TypeError:  is not a valid URL."]
Will check further on this issue.
Flags: needinfo?(gasolin)
Fixed by listen `beforeunload` in _startUI when visible, so the event won't accidently effect other page's test.

Discussed the changes (moved registerNewTelemetrySession and beforeunload into _startUI) with fischer. Will land again after try green.
Pushed by flin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b78d2b24b741
Data schema change for onboarding telemetry;r=Fischer,francois
Flags: needinfo?(gasolin)
Saw docshell leak test fail and can't reproduce locally with wcpan's help.
Test also passed with try herder's `one click loan` machine.


As comment 183 I tried to use `unload` event instead and it solved the test failure.
So the test failure might be related to the usage of `beforeunload` event.


I'm trying to add several cases with try server to scope the root cause:

* use unload event
https://reviewboard.mozilla.org/r/194564/diff/25

* comment out contents in destroy
https://reviewboard.mozilla.org/r/211686/diff/2/

Will do more tests when tree gecko is open.
Flags: needinfo?(gasolin)
Depends on: 1429652
> I'm trying to add several cases with try server to scope the root cause:
> 
> * use unload event
> https://reviewboard.mozilla.org/r/194564/diff/25
> 

try green with unload
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3430b0be528&selectedJob=155336782

> * comment out contents in destroy
> https://reviewboard.mozilla.org/r/211686/diff/3
> 

Try Fail with beforeunload event (removed the callback functions content to eliminate effect from the other sentences)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5cc99d79ed6

So the docshell leak very likely comes from the beforeunload event. File bug 1429652 to track this.
Since we can't use beforeunload due to docshell leak, I suggest using `unload` instead and land the patch.

The tradeoff is we will not able to access dom elements during unload,
we will still get onboarding-session, but in `destroy` function we won't get overlay-session / notification-session event when user close the tab. (we will still get overlay-session / notification-session events when the destory function is triggered by resize event)

Here's the changeset https://reviewboard.mozilla.org/r/198382/diff/57-58/
Fischer, how do you think?
Flags: needinfo?(fliu)
(In reply to Fred Lin [:gasolin] from comment #194)
> Since we can't use beforeunload due to docshell leak, I suggest using
> `unload` instead and land the patch.
> 
> The tradeoff is we will not able to access dom elements during unload,
> we will still get onboarding-session, but in `destroy` function we won't get
> overlay-session / notification-session event when user close the tab. (we
> will still get overlay-session / notification-session events when the
> destory function is triggered by resize event)
> 
> Here's the changeset https://reviewboard.mozilla.org/r/198382/diff/57-58/
> Fischer, how do you think?
The correctness of events are important otherwise we just lose the meaning of this patch.

(In reply to Fred Lin [:gasolin] from comment #192)
> Try Fail with beforeunload event (removed the callback functions content to eliminate effect from the other sentences)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5cc99d79ed6
> 
> So the docshell leak very likely comes from the beforeunload event. File bug 1429652 to track this.
From the log of this failure case, there are a bunch of "TypeError: can't access dead object" errors. Please have a look at this to see if something is connected.
[1] https://treeherder.mozilla.org/logviewer.html#?job_id=155479083&repo=try&lineNumber=10791
[2] https://treeherder.mozilla.org/logviewer.html#?job_id=155479083&repo=try&lineNumber=10281
[3] https://treeherder.mozilla.org/logviewer.html#?job_id=155479083&repo=try&lineNumber=10351
Flags: needinfo?(fliu)
(In reply to Fischer [:Fischer] from comment #195)
> (In reply to Fred Lin [:gasolin] from comment #194)
> > Since we can't use beforeunload due to docshell leak, I suggest using
> > `unload` instead and land the patch.
> > 
> > The tradeoff is we will not able to access dom elements during unload,
> > we will still get onboarding-session, but in `destroy` function we won't get
> > overlay-session / notification-session event when user close the tab. (we
> > will still get overlay-session / notification-session events when the
> > destory function is triggered by resize event)
> > 
> > Here's the changeset https://reviewboard.mozilla.org/r/198382/diff/57-58/
> > Fischer, how do you think?
> The correctness of events are important otherwise we just lose the meaning
> of this patch.
> 

I agree keep the correctness of events are important. Though I found current implementation also does not send overlay-/notification-session-end while unload
https://searchfox.org/mozilla-central/source/browser/extensions/onboarding/content/onboarding.js#710

Besides these 2 events (which does not send when unload), this patch did many other enhancements. 
It will be a loss if we can't catch these more accurate data in v59. 
Could you reconsider if we can land the patch and fix the `beforeunload` case in the followup?
Flags: needinfo?(fliu)
(In reply to Fred Lin [:gasolin](OOO till 1/18) from comment #196)
> (In reply to Fischer [:Fischer] from comment #195)
> > (In reply to Fred Lin [:gasolin] from comment #194)
> > > Since we can't use beforeunload due to docshell leak, I suggest using
> > > `unload` instead and land the patch.
> > > 
> > > The tradeoff is we will not able to access dom elements during unload,
> > > we will still get onboarding-session, but in `destroy` function we won't get
> > > overlay-session / notification-session event when user close the tab. (we
> > > will still get overlay-session / notification-session events when the
> > > destory function is triggered by resize event)
> > > 
> > > Here's the changeset https://reviewboard.mozilla.org/r/198382/diff/57-58/
> > > Fischer, how do you think?
> > The correctness of events are important otherwise we just lose the meaning
> > of this patch.
> > 
> 
> I agree keep the correctness of events are important. Though I found current
> implementation also does not send overlay-/notification-session-end while
> unload
> https://searchfox.org/mozilla-central/source/browser/extensions/onboarding/
> content/onboarding.js#710
> 
> Besides these 2 events (which does not send when unload), this patch did
> many other enhancements. 
> It will be a loss if we can't catch these more accurate data in v59. 
> Could you reconsider if we can land the patch and fix the `beforeunload` case in the followup?
This is a bad news. The old telemetry is missing these pings as well so the same mistake happens again here.
Since lots of effort has been put on the new telemetry and just one error left to fix, and the error looks like not the one unable to fix or workaround, please try to work on this.

(In reply to Fred Lin [:gasolin](OOO till 1/18) from comment #192)
> Try Fail with beforeunload event (removed the callback functions content to
> eliminate effect from the other sentences)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5cc99d79ed6
> 
> So the docshell leak very likely comes from the beforeunload event. File bug 1429652 to track this.
From this failure case, even not executing the below tasks:
  ```
  +    // this._clearPrefObserver();
  +    // this._overlayIcon.remove();
  +    // if (this._overlay) {
  +    //   // send overlay-session telemetry
  +    //   this.hideOverlay();
  +    //   this._overlay.remove();
  +    // }
  +    // if (this._notificationBar) {
  +    //   // send notification-session telemetry
  +    //   this.hideNotification();
  +    //   this._notificationBar.remove();
  +    // }
  +    // this._tourItems = this._tourPages =
  +    // this._overlayIcon = this._overlay = this._notificationBar = null;
  ```
the error still happens.
So that means 
 A: `this._overlayIcon.dispatchEvent(new this._window.CustomEvent("Agent:Destroy"));`
 B: Sending `"onboarding-session-end"`
may be the cause.
Maybe we could run try to see which line causing the issue and find a fix/workaround for it.
Flags: needinfo?(fliu)
(In reply to Fischer [:Fischer] from comment #198)
> Created attachment 8942582 [details] [diff] [review]
> Bug-1413830-Data-schema-change-for-onboarding-telemetry.patch
Notice: Please check-in this patch.

This patch is based on the patch [1] from the comment 193, then plus the workaround for the browser_aboutHome_search_telemetry.js test leakage issue.

In this patch, the telemetry pings are move into the beforeunload event so as to ensure pings get sent and do `destroy`(the jobs to remove elements) on unload.

The several Try runs[2][3][4] show the results look good, no browser_aboutHome_search_telemetry.js leakage issue there.

[1] attachment 8927140 [details]: Bug 1413830 - Data schema change for onboarding telemetry (From https://reviewboard.mozilla.org/r/198384/diff/50-51/)
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=b04d380e7712d0e69ededfe40a8b268e08067e80
[3] https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f489dc430df229a7618fb3d50cfc3d9a83b27b0
[4] https://treeherder.mozilla.org/#/jobs?repo=try&revision=d70f01ca7ced700e15032c1249c6f473bd6a4c85
Keywords: checkin-needed
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

Although Bug-1413830-Data-schema-change-for-onboarding-telemetry.patch[1] is based on this patch, in order to avoid confusion and mistake while checking in codes. Mark this attachment obsolete. However, the review approvals for this patch still count.

[1] attachment 8942582 [details] [diff] [review]: Bug-1413830-Data-schema-change-for-onboarding-telemetry.patch
Attachment #8927140 - Attachment is obsolete: true
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14a96e730a86
Data schema change for onboarding telemetry;r=fischer,francois
Keywords: checkin-needed
Comment on attachment 8927140 [details]
Bug 1413830 - Data schema change for onboarding telemetry;

Restore since we got the right check-in order, see Comment 201
Attachment #8927140 - Attachment is obsolete: false
https://hg.mozilla.org/mozilla-central/rev/14a96e730a86
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.