Closed Bug 1418167 Opened 7 years ago Closed 7 years ago

validate data before send 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

Details

(Whiteboard: [onboarding-telemetry])

Attachments

(1 file)

Bug 1418167 - validate data before send for onboarding telemetry;
59 bytes, text/x-review-board-request
Fischer
: review+
Details 
== clone from https://bugzilla.mozilla.org/show_bug.cgi?id=1413830#c42 ==

Its 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.
The patch implemented basic validation to check each SESSION/EVENT column and report the incorrect fields.

I also removed session event's `tour_id` column since it is not used in our dashboard query and we will remove that in new telemetry.

Will add per event validation in bug 1413830
Blocks: 1388648
Comment on attachment 8929287 [details]
Bug 1418167 - validate data before send for onboarding telemetry;

https://reviewboard.mozilla.org/r/200600/#review205828

Need some modifications on the schema checkers

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:21
(Diff revision 2)
>  XPCOMUtils.defineLazyServiceGetter(this, "gUUIDGenerator",
>    "@mozilla.org/uuid-generator;1", "nsIUUIDGenerator");
>  
> +// Validate the content has non-empty string
> +function hasString(str) {
> +  return str.length > 0;

typeof str == "string" && ...

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:26
(Diff revision 2)
> +  return str.length > 0;
> +}
> +
> +// Validate the content is a number
> +function isNumber(num) {
> +  return typeof num === "number";

```
function isInteger(i) {
  return Number.isInteger(i);
}
```
We currently only need the integer data, right?

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:36
(Diff revision 2)
> +  {key: "addon_version", validator: hasString},
> +  {key: "category", validator: hasString},
> +  {key: "event", validator: hasString},
> +  {key: "page", validator: str => ["about:newtab", "about:home"].includes(str)},
> +  {key: "session_begin", validator: isNumber},
> +  {key: "session_end", validator: isNumber},

`session_begin` and `session_end` are not part of the schema. They are internal params to build pings so move them to other place to check

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:38
(Diff revision 2)
> +  {key: "event", validator: hasString},
> +  {key: "page", validator: str => ["about:newtab", "about:home"].includes(str)},
> +  {key: "session_begin", validator: isNumber},
> +  {key: "session_end", validator: isNumber},
> +  {key: "session_id", validator: hasString},
> +  {key: "tour_source", validator: str => ["default", "watermark"].includes(str)},

`tour_source` should no longer exist at least in the new schema right? Please seperate old columns from new columns. During the old and new schema transition, could be like:
1. Check the common schema across the new/old schema
2. Check the new schema if we are now sending to the new schame back-end table

We probably don't want to check the old columns because they are going to be retired (save some effort on that) and we are not worse off without doing that.

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:48
(Diff revision 2)
> +const EVENT_SCHEMA = [
> +  {key: "addon_version", validator: hasString},
> +  {key: "category", validator: hasString},
> +  {key: "event", validator: hasString},
> +  {key: "impression", validator: isNumber},
> +  {key: "page", validator: str => ["about:newtab", "about:home"].includes(str)},

We should share checkers like `hasString`, `isInteger` etc
Attachment #8929287 - Flags: review?(fliu)
Comment on attachment 8929287 [details]
Bug 1418167 - validate data before send for onboarding telemetry;

https://reviewboard.mozilla.org/r/200600/#review205828

> `session_begin` and `session_end` are not part of the schema. They are internal params to build pings so move them to other place to check

Mistake, they are part of schema....
Comment on attachment 8929287 [details]
Bug 1418167 - validate data before send for onboarding telemetry;

https://reviewboard.mozilla.org/r/200600/#review205828

> typeof str == "string" && ...

updated

> ```
> function isInteger(i) {
>   return Number.isInteger(i);
> }
> ```
> We currently only need the integer data, right?

updated

> `tour_source` should no longer exist at least in the new schema right? Please seperate old columns from new columns. During the old and new schema transition, could be like:
> 1. Check the common schema across the new/old schema
> 2. Check the new schema if we are now sending to the new schame back-end table
> 
> We probably don't want to check the old columns because they are going to be retired (save some effort on that) and we are not worse off without doing that.

as offline discussion, I'll add a NEW_TABLE flag and make these schema only check the new telemetry. We can remove the flag and old OLD_EVENT_WHITELIST when server support the new table

> We should share checkers like `hasString`, `isInteger` etc

add `oneOf` function to do so
Comment on attachment 8929287 [details]
Bug 1418167 - validate data before send for onboarding telemetry;

https://reviewboard.mozilla.org/r/200600/#review206230

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:46
(Diff revision 10)
> +// Basic validators for session events
> +// client_id, locale are added by PingCentre, IP is added by server
> +// so no need check these column here
> +const SESSION_SCHEMA = {
> +  addon_version: hasString,
> +  category: hasString,

`category` like `page` has specific valid value so should define that as well.
p.s I believe the onboarding-session doesn't belong to "overlay-interactions" or "notification-interactions". In fact it encompasses these 2.

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:48
(Diff revision 10)
> +// so no need check these column here
> +const SESSION_SCHEMA = {
> +  addon_version: hasString,
> +  category: hasString,
> +  event: hasString,
> +  page: oneOf(["about:newtab", "about:home"]),

`oneOf` is still not sharing the same check condition. Please go like
```
isValidPage(page) {
  // check is "about:newtab" or "about:home"
}
```

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:54
(Diff revision 10)
> +  parent_session_id: hasString,
> +  root_session_id: hasString,
> +  session_begin: isInteger,
> +  session_end: isInteger,
> +  session_id: hasString,
> +  tour_type: oneOf(["new", "update"]),

Please go such as `isValidTourType`

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:61
(Diff revision 10)
> +};
> +
> +// Basic validators for session events
> +// client_id, locale are added by PingCentre, IP is added by server
> +// so no need check these column here
> +const EVENT_SCHEMA = {

nit: Could we name it "COMMON_EVENT_SCHEMA" or "BASIC_EVENT_SCHEMA"

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:67
(Diff revision 10)
> +  addon_version: hasString,
> +  bubble_state: isEmptyString,
> +  category: hasString,
> +  current_tour_id: hasString,
> +  logo_state: isEmptyString,
> +  notification_impression: str => str === -1,

this `notification_impression` is saved in string type??

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:76
(Diff revision 10)
> +  root_session_id: hasString,
> +  parent_session_id: hasString,
> +  timestamp: isInteger,
> +  tour_type: oneOf(["new", "update"]),
> +  type: hasString,
> +  width: str => str === -1,

From this, does it imply that later pings which needs to send width data will override this `width` checker later?
If so, I would suggest for this type of data do:
```
width: () => throw "Must define the `width` data checker per ping because it is not the same for all pings";
```
In this way, our code self-documents the requiremnts of pings.

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:94
(Diff revision 10)
>   * of analytics engineer's request.
>   */
>  const EVENT_WHITELIST = {
>    // track when click the notification close button
> +  "notification-close-button-click": {
> +    topic: "firefox-onboarding-event",

We need new topics for the new events: https://bugzilla.mozilla.org/show_bug.cgi?id=1413830#c50.
Please leave a "TODO: ...." comment for that

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:225
(Diff revision 10)
>        session_id,
>        tour_type,
>      };
>    },
>  
>    process(data) {

The `registerNewTelemetrySession` and payload part down there and many places are not 100% guaranteed compatible with the new schame.  Since we have been using the `NEW_TABLE` flag, I was thinking go like
```
if (NEW_TABLE) {
  // Then we can rethink and modify the codes for the new schame thoroughly without the worry of braking the old pings
  throw "Not yet implement the `process` and `_send` method for the new schema, should not enable `NEW_TABLE`"
} else {
  // Just rename the method and we don't have to touch it inside
  // and do the same for `_sendOldPings`
  this._processOldPings(data)
}
```
This is a bit ugly but quick, clear and very low risk. I think we can live with that for 1~2 weeks until the new schema goes online. How do you think?

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:324
(Diff revision 10)
>            tour_source,
>            tour_type,
> -        }, {filter: ONBOARDING_ID});
> +        };
> +        this._validatePayload(payload);
> +        this.sessionProbe && this.sessionProbe.sendPing(payload,
> +          {filter: ONBOARDING_ID});

Will `{filter: ONBOARDING_ID}` be affected by the new/old schema, like topic? Could you check with the Ping Center team?
Not necessarily handle in this bug, can do in Bug 1413830. But we must make sure that so the old/new data won't get mixed on the server.

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:362
(Diff revision 10)
> +    if (!validators) {
> +      throw new Error(`Event ${event} without validators should not be sent`);
> +    }
> +    let results = this._validateColumns(payload, validators);
> +    if (!results.validation_result) {
> +      throw new Error(`Event ${event} contains incorrect data , should not be sent ${JSON.stringify(results)}`);

nit: `Event ${event} contains incorrect data: ${JSON.stringify(results)}, should not be sent`

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:367
(Diff revision 10)
> +      throw new Error(`Event ${event} contains incorrect data , should not be sent ${JSON.stringify(results)}`);
> +    }
> +  },
> +
> +  // Per column validation
> +  _validateColumns(payload, validators) {

Could move this into `validatePayload` because it is not really big?

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:370
(Diff revision 10)
> +
> +  // Per column validation
> +  _validateColumns(payload, validators) {
> +    let results = {};
> +    for (let key of Object.keys(validators)) {
> +      if (payload[key]) {

What if `payload[key] == 0` and 0 is valid data?

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:380
(Diff revision 10)
> +      } else {
> +        results[key] = false;
> +        results.validation_result = false;
> +      }
> +    }
> +    if (!results.validation_result) {

Don't mix `validation_result` with `results`. `results` only carrys the column validation result.
So maybe do
```
if (failed) {
  // ............
}
```
Attachment #8929287 - Flags: review?(fliu)
Comment on attachment 8929287 [details]
Bug 1418167 - validate data before send for onboarding telemetry;

https://reviewboard.mozilla.org/r/200600/#review206230

> `category` like `page` has specific valid value so should define that as well.
> p.s I believe the onboarding-session doesn't belong to "overlay-interactions" or "notification-interactions". In fact it encompasses these 2.

implement `isValidCategory`.
let's put the category blank when sending `onboarding-session`

> nit: Could we name it "COMMON_EVENT_SCHEMA" or "BASIC_EVENT_SCHEMA"

updated to BASIC_EVENT_SCHEMA/SESSION_SCHEMA

> this `notification_impression` is saved in string type??

updated

> From this, does it imply that later pings which needs to send width data will override this `width` checker later?
> If so, I would suggest for this type of data do:
> ```
> width: () => throw "Must define the `width` data checker per ping because it is not the same for all pings";
> ```
> In this way, our code self-documents the requiremnts of pings.

No we don't have to define per ping. the BASIC_EVENT_SCHEMA should define validator for common cases. For most of events we should send -1 for width. 
Then we only need override the width validator for 3 defined actions (onboarding-logo-click, onboarding-noshow, overlay-disapear).

> We need new topics for the new events: https://bugzilla.mozilla.org/show_bug.cgi?id=1413830#c50.
> Please leave a "TODO: ...." comment for that

updated to `firefox-onboarding-event2` `firefox-onboarding-session2` directly since we have a NEW_TABLE

> The `registerNewTelemetrySession` and payload part down there and many places are not 100% guaranteed compatible with the new schame.  Since we have been using the `NEW_TABLE` flag, I was thinking go like
> ```
> if (NEW_TABLE) {
>   // Then we can rethink and modify the codes for the new schame thoroughly without the worry of braking the old pings
>   throw "Not yet implement the `process` and `_send` method for the new schema, should not enable `NEW_TABLE`"
> } else {
>   // Just rename the method and we don't have to touch it inside
>   // and do the same for `_sendOldPings`
>   this._processOldPings(data)
> }
> ```
> This is a bit ugly but quick, clear and very low risk. I think we can live with that for 1~2 weeks until the new schema goes online. How do you think?

The process/send is separated for new/old table

> Will `{filter: ONBOARDING_ID}` be affected by the new/old schema, like topic? Could you check with the Ping Center team?
> Not necessarily handle in this bug, can do in Bug 1413830. But we must make sure that so the old/new data won't get mixed on the server.

will check in bug 1413830

> nit: `Event ${event} contains incorrect data: ${JSON.stringify(results)}, should not be sent`

updated

> Could move this into `validatePayload` because it is not really big?

merged into `_validatePayload`

> What if `payload[key] == 0` and 0 is valid data?

check via `payload[key] !== undefined`

> Don't mix `validation_result` with `results`. `results` only carrys the column validation result.
> So maybe do
> ```
> if (failed) {
>   // ............
> }
> ```

updated
Comment on attachment 8929287 [details]
Bug 1418167 - validate data before send for onboarding telemetry;

https://reviewboard.mozilla.org/r/200600/#review206230

> No we don't have to define per ping. the BASIC_EVENT_SCHEMA should define validator for common cases. For most of events we should send -1 for width. 
> Then we only need override the width validator for 3 defined actions (onboarding-logo-click, onboarding-noshow, overlay-disapear).

>  the BASIC_EVENT_SCHEMA should define validator for common cases

Yes, BASIC_EVENT_SCHEMA should define validator for common cases. Then why do we still define a "particualr" validator for non common case?

> Then we only need override the width validator for 3 defined actions (onboarding-logo-click, onboarding-noshow, overlay-disapear)

So are we going to override the width for 3 events forever? Never ever new case come?

We have to make the schema validations clear and explicit. That is the value of it.
Comment on attachment 8929287 [details]
Bug 1418167 - validate data before send for onboarding telemetry;

https://reviewboard.mozilla.org/r/200600/#review206316
Attachment #8929287 - Flags: review?(fliu)
Comment on attachment 8929287 [details]
Bug 1418167 - validate data before send for onboarding telemetry;

https://reviewboard.mozilla.org/r/200600/#review206230

> >  the BASIC_EVENT_SCHEMA should define validator for common cases
> 
> Yes, BASIC_EVENT_SCHEMA should define validator for common cases. Then why do we still define a "particualr" validator for non common case?
> 
> > Then we only need override the width validator for 3 defined actions (onboarding-logo-click, onboarding-noshow, overlay-disapear)
> 
> So are we going to override the width for 3 events forever? Never ever new case come?
> 
> We have to make the schema validations clear and explicit. That is the value of it.

As discussion offline, I'll put `isInteger` instead
Comment on attachment 8929287 [details]
Bug 1418167 - validate data before send for onboarding telemetry;

https://reviewboard.mozilla.org/r/200600/#review206230

> As discussion offline, I'll put `isInteger` instead

Sorry, I don't remember the discussion ends with using `isInteger` instead
Comment on attachment 8929287 [details]
Bug 1418167 - validate data before send for onboarding telemetry;

https://reviewboard.mozilla.org/r/200600/#review206648
Attachment #8929287 - Flags: review?(fliu)
(In reply to Fischer [:Fischer] from comment #30)
> Comment on attachment 8929287 [details]
> Bug 1418167 - validate data before send for onboarding telemetry;
> 
> https://reviewboard.mozilla.org/r/200600/#review206230
> 
> > As discussion offline, I'll put `isInteger` instead
> 
> Sorry, I don't remember the discussion ends with using `isInteger` instead

After second thought, Now all optional columns in BASIC_SCHEMA will throw Error with `definePerPing` will message `Must define the '${column}' validator per ping because it is not the same for all pings`
So it's now different from the previous implementation. Please kindly review it again.
Flags: needinfo?(fliu)
Comment on attachment 8929287 [details]
Bug 1418167 - validate data before send for onboarding telemetry;

https://reviewboard.mozilla.org/r/200600/#review206696

Saw some uncertainty about the schema. Would you check again?

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:64
(Diff revision 20)
> +// client_id, locale are added by PingCentre, IP is added by server
> +// so no need check these column here
> +const BASIC_SESSION_SCHEMA = {
> +  addon_version: hasString,
> +  category: definePerPing("category"),
> +  event: hasString,

still need "event" column?
Comment on attachment 8929287 [details]
Bug 1418167 - validate data before send for onboarding telemetry;

https://reviewboard.mozilla.org/r/200600/#review206696

> still need "event" column?

thanks for finding that, removed
Flags: needinfo?(fliu)
Comment on attachment 8929287 [details]
Bug 1418167 - validate data before send for onboarding telemetry;

https://reviewboard.mozilla.org/r/200600/#review207200

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:112
(Diff revision 22)
>  const EVENT_WHITELIST = {
>    // track when click the notification close button
> +  "notification-close-button-click": {
> +    topic: "firefox-onboarding-event2",
> +    category: "notification-interactions",
> +    validators: BASIC_EVENT_SCHEMA,

Some events have per-ping columns overridden, some not. Your plan is to do per-ping columns in this bug or the following bugs? No preference, just let me know the plan

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:132
(Diff revision 22)
> +  },
> +  // track when notification is shown
> +  "notification-session-begin": {topic: "internal"},
> +  // track when the notification closed
> +  "notification-session-end": {
> +    topic: "firefox-onboarding-session2",

"notification-session-end" belongs to an internal topic.

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:176
(Diff revision 22)
> +  },
> +  // track when the overlay is shown
> +  "overlay-session-begin": {topic: "internal"},
> +  // track when the overlay is closed
> +  "overlay-session-end": {
> +    topic: "firefox-onboarding-session2",

internal topic
Attachment #8929287 - Flags: review?(fliu)
Comment on attachment 8929287 [details]
Bug 1418167 - validate data before send for onboarding telemetry;

https://reviewboard.mozilla.org/r/200600/#review207200

> "notification-session-end" belongs to an internal topic.

The old way put it under "firefox-onboarding-session" though, now we are doing schema checks for out-going pings so better make internal and external topic clear.
Comment on attachment 8929287 [details]
Bug 1418167 - validate data before send for onboarding telemetry;

https://reviewboard.mozilla.org/r/200600/#review207200

> Some events have per-ping columns overridden, some not. Your plan is to do per-ping columns in this bug or the following bugs? No preference, just let me know the plan

Yes, the plan is to do per-ping columns in the following bugs. But since session schema is simpler and only have 1 column need to be defined per pings (`category`), so I defined those session schema  in this patch.

> The old way put it under "firefox-onboarding-session" though, now we are doing schema checks for out-going pings so better make internal and external topic clear.

Define as "internal" will drop us into "internal" processing and we need to call `sendPing` several times there
https://searchfox.org/mozilla-central/source/browser/extensions/onboarding/OnboardingTelemetry.jsm#106
We need to do the check in sendPing again when we want to send the session events

I'd like to add a new topic "alias" so the "internal" processing is not triggered and we clearly know the event name will be changed and will do the validation with other event name

> internal topic

changed to "alias"
Comment on attachment 8929287 [details]
Bug 1418167 - validate data before send for onboarding telemetry;

https://reviewboard.mozilla.org/r/200600/#review207200

> Define as "internal" will drop us into "internal" processing and we need to call `sendPing` several times there
> https://searchfox.org/mozilla-central/source/browser/extensions/onboarding/OnboardingTelemetry.jsm#106
> We need to do the check in sendPing again when we want to send the session events
> 
> I'd like to add a new topic "alias" so the "internal" processing is not triggered and we clearly know the event name will be changed and will do the validation with other event name

I did more experiment on Bug 1413830 and found add `alias` topic is not a good idea.

Adding internal processing like

```
case "overlay-session-end":
          event = "overlay-session";
          topic = "firefox-onboarding-session2";
          this._sendPings(topic, data);
          break;
```

is simpler and not need to change `_sendPings` a lot. 

So I'll use `internal` here.
Blocks: 1419996
Comment on attachment 8929287 [details]
Bug 1418167 - validate data before send for onboarding telemetry;

https://reviewboard.mozilla.org/r/200600/#review207254

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:122
(Diff revision 28)
> +  "notification-cta-click": {
> +    topic: "firefox-onboarding-event2",
> +    category: "notification-interactions",
> +    validators: BASIC_EVENT_SCHEMA,
> +  },
> +  // The real event name send to the server, alias of notification-session-end

Remove "alias of notification-session-end" because notification-session is not equal to notification-session-end. They are not aliases to each other.

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:136
(Diff revision 28)
> +  "notification-session-begin": {topic: "internal"},
> +  // track when the notification closed
> +  "notification-session-end": {topic: "internal"},
> +  // init onboarding session with session_key and page url
> +  "onboarding-register-session": {topic: "internal"},
> +  // The real event name send to the server, alias of onboarding-session-end

Remove alias part

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:146
(Diff revision 28)
> +      category: isEmptyString,
> +    }),
> +  },
> +  // track when the onboarding script inited
> +  "onboarding-session-begin": {topic: "internal"},
> +  // track when the onboarding script destoryed

Just remembered the new onboarding session definition is from one about page load to unload. Please find Product for a short meeting to have this part checked and aligned again.
Attachment #8929287 - Flags: review?(fliu)
> 
> Just remembered the new onboarding session definition is from one about page
> load to unload. Please find Product for a short meeting to have this part
> checked and aligned again.

Hi Cindy,

the last time we decide to track the onboarding-session from the tab is visible to the tab unload (close the tab), is that correct?

(FYR currently we track onboarding-session when onboarding module is inited and send the event when onboarding module is destoryed in any situation(includes unload the tab or resize screen width to smaller than the threshold))
Flags: needinfo?(chsiang)
Yes tab load and unload.
Flags: needinfo?(chsiang)
Comment on attachment 8929287 [details]
Bug 1418167 - validate data before send for onboarding telemetry;

https://reviewboard.mozilla.org/r/200600/#review207254

> Remove "alias of notification-session-end" because notification-session is not equal to notification-session-end. They are not aliases to each other.

updated

> Remove alias part

updated

> Just remembered the new onboarding session definition is from one about page load to unload. Please find Product for a short meeting to have this part checked and aligned again.

As discussed I'll put all events (existing and new events) and the schema in this bug so we can go through the descrition with Cindy to make sure we are on the same page.
Comment on attachment 8929287 [details]
Bug 1418167 - validate data before send for onboarding telemetry;

https://reviewboard.mozilla.org/r/200600/#review207994

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:50
(Diff revision 30)
> +  return num === -1;
> +}
> +
> +// Validate the category value is within the list
> +function isValidCategory(category) {
> +  return ["onboarding-interactions", "overlay-interactions", "notification-interactions"]

What's meaning of "onboarding-interactions"?

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:91
(Diff revision 30)
> +// Basic validators for session events
> +// client_id, locale are added by PingCentre, IP is added by server
> +// so no need check these column here
> +const BASIC_SESSION_SCHEMA = {
> +  addon_version: hasString,
> +  category: isValidCategory,

Isn't this defined per ping?

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:145
(Diff revision 30)
> +    validators: Object.assign({}, BASIC_EVENT_SCHEMA, {
> +      bubble_state: isValidBubbleState,
> +      current_tour_id: hasString,
> +      logo_state: isValidLogoState,
> +      notification_impression: isPositiveInteger,
> +      notification_state: isEmptyString,

This notification_state should not be empty? notification has state when appearing

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:159
(Diff revision 30)
> +    validators: Object.assign({}, BASIC_EVENT_SCHEMA, {
> +      bubble_state: isValidBubbleState,
> +      current_tour_id: hasString,
> +      logo_state: isValidLogoState,
> +      notification_impression: isPositiveInteger,
> +      notification_state: isEmptyString,

This notification_state should not be empty?

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:160
(Diff revision 30)
> +      bubble_state: isValidBubbleState,
> +      current_tour_id: hasString,
> +      logo_state: isValidLogoState,
> +      notification_impression: isPositiveInteger,
> +      notification_state: isEmptyString,
> +      target_tour_id: isEmptyString,

should not be empty?

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:173
(Diff revision 30)
> +    validators: Object.assign({}, BASIC_EVENT_SCHEMA, {
> +      bubble_state: isValidBubbleState,
> +      current_tour_id: hasString,
> +      logo_state: isValidLogoState,
> +      notification_impression: isPositiveInteger,
> +      notification_state: isEmptyString,

should not be empty?

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:175
(Diff revision 30)
> +      current_tour_id: hasString,
> +      logo_state: isValidLogoState,
> +      notification_impression: isPositiveInteger,
> +      notification_state: isEmptyString,
> +      target_tour_id: hasString,
> +      width: isPositiveInteger,

why only this one is positive?

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:197
(Diff revision 30)
> +    validators: Object.assign({}, BASIC_EVENT_SCHEMA, {
> +      bubble_state: isValidBubbleState,
> +      current_tour_id: isEmptyString,
> +      logo_state: isValidLogoState,
> +      notification_impression: isMinusOne,
> +      notification_state: isValidNotificationState,

we got notification_state but notification_impression is -1?

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:203
(Diff revision 30)
> +      target_tour_id: isEmptyString,
> +      width: isPositiveInteger,
> +    }),
> +  },
> +  // track when in a smaller screen that we won't show the onboarding
> +  "onboarding-noshow": {

This belongs to which category, overlay-interactions or notification-interactions? Since no interactions at all

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:219
(Diff revision 30)
> +    }),
> +  },
> +  // init onboarding session with session_key and page url
> +  "onboarding-register-session": {topic: "internal"},
> +  // The real event name send to the server
> +  "onboarding-session": {

This belongs to which category, overlay-interactions or notification-interactions?

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:238
(Diff revision 30)
> +      bubble_state: isEmptyString,
> +      current_tour_id: hasString,
> +      logo_state: isEmptyString,
> +      notification_impression: isMinusOne,
> +      notification_state: isEmptyString,
> +      target_tour_id: isEmptyString,

should not be empty?

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:252
(Diff revision 30)
> +      bubble_state: isEmptyString,
> +      current_tour_id: hasString,
> +      logo_state: isEmptyString,
> +      notification_impression: isMinusOne,
> +      notification_state: isEmptyString,
> +      target_tour_id: isEmptyString,

remembered Product has said this should be classfied as empty. Check this out later.

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:285
(Diff revision 30)
> +      target_tour_id: isEmptyString,
> +      width: isMinusOne,
> +    }),
> +  },
> +  // track when open the overlay and resized to a smaller window
> +  "overlay-disapear": {

should we use overly-disappear-for-resize?

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:295
(Diff revision 30)
> +      current_tour_id: isEmptyString,
> +      logo_state: isEmptyString,
> +      notification_impression: isMinusOne,
> +      notification_state: isEmptyString,
> +      target_tour_id: isEmptyString,
> +      width: isMinusOne,

Should we bring width?

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:298
(Diff revision 30)
> +      notification_state: isEmptyString,
> +      target_tour_id: isEmptyString,
> +      width: isMinusOne,
> +    }),
> +  },
> +  // track when click or auto select the overlay navigate item

What is auto select?

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:312
(Diff revision 30)
> +      notification_state: isEmptyString,
> +      target_tour_id: hasString,
> +      width: isMinusOne,
> +    }),
> +  },
> +  // The real event name send to the server

Should we change to a meaningful comment like other pings?
Attachment #8929287 - Flags: review?(fliu)
Comment on attachment 8929287 [details]
Bug 1418167 - validate data before send for onboarding telemetry;

https://reviewboard.mozilla.org/r/200600/#review207994

> What's meaning of "onboarding-interactions"?

interactions besides the overlay, notification-interactions are categorized as onboarding-interactions.  Now are `onboarding-session` and  `onboarding-noshow-smallscreen`

> Isn't this defined per ping?

every session ping should have a category now, so we can use `isValidCategory` to check that

> This notification_state should not be empty? notification has state when appearing

after discussion we will send notification_state for the notification-appear event.

> This notification_state should not be empty?

after discussion we will send notification_state for the notification-close-button-click event.

> should not be empty?

after discussion we will send notification_state for the notification-close-button-click event.

> should not be empty?

after discussion we will send notification_state for the notification-cta-click event.

> why only this one is positive?

after discussion we will send width for every event

> we got notification_state but notification_impression is -1?

after discussion we decide to keep send notification_state but not send notification_impression or target_tour_id

> This belongs to which category, overlay-interactions or notification-interactions? Since no interactions at all

after discussion we decide to put it into `onboarding-interactions` category

> This belongs to which category, overlay-interactions or notification-interactions?

after discussion we decide to put it into `onboarding-interactions` category

> should not be empty?

after discussion we decide to send target_tour_id for overlay-close-button-click event

> remembered Product has said this should be classfied as empty. Check this out later.

after discussion we decide to send target_tour_id for overlay-close-outside-click event

> should we use overly-disappear-for-resize?

we decide to name it as `overlay-disapear-resize`

> Should we bring width?

we'll bring width for every event pings

> What is auto select?

update string to remove the auto select part

> Should we change to a meaningful comment like other pings?

updated according to offline discussion
Comment on attachment 8929287 [details]
Bug 1418167 - validate data before send for onboarding telemetry;

https://reviewboard.mozilla.org/r/200600/#review208360

::: browser/extensions/onboarding/OnboardingTelemetry.jsm:125
(Diff revision 36)
> +  type: hasString,
> +  width: isPositiveInteger,
> +};
> +
>  /**
>   * We send 2 kinds (firefox-onboarding-event, firefox-onboarding-session) of pings to ping centre

Now the below is "firefox-onboarding-event2, firefox-onboarding-session2"
Attachment #8929287 - Flags: review?(fliu) → review+
Comment on attachment 8929287 [details]
Bug 1418167 - validate data before send for onboarding telemetry;

https://reviewboard.mozilla.org/r/200600/#review208360

> Now the below is "firefox-onboarding-event2, firefox-onboarding-session2"

Yes, updated
Thanks for review! 

Fixed some typo before land https://reviewboard.mozilla.org/r/200600/diff/36-38/
Pushed by flin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a19a802032da
validate data before send for onboarding telemetry;r=Fischer
https://hg.mozilla.org/mozilla-central/rev/a19a802032da
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: