mozilla / ping-centre Goto Github PK

A user should not need to tell the client which schema they want to use. A schema should be applied based on the topic.

DISCLAIMER: This is for the current [email protected] on npm.

$ cat ./node_modules/ping-centre/package.json | grep _id
  "_id": "[email protected]",

Steps to reproduce:

1. Create a new, empty node project.
2. npm i ping-centre@latest -S
3. tree node_modules/ping-centre

Actual results:

node_modules/ping-centre
├── LICENSE
├── Makefile
├── README.md
├── example.js
├── package.json
├── ping-centre.js
├── schemas/
│   ├── commonSchema.js
│   └── sample.js
└── test/
    ├── common.js
    └── ping-centre.js

2 directories, 10 files

$ ls -lashR node_modules/ping-centre/

   1.6K Nov 28 07:04 .eslintrc
    22B Nov 28 07:04 .npmignore
    16K Dec  1 09:55 LICENSE
    63B Nov 28 07:04 Makefile
   3.4K Dec  1 09:48 README.md
   532B Nov 28 07:04 example.js
   2.5K Feb  6 21:29 package.json
   1.5K Dec  1 09:48 ping-centre.js
   249B Nov 28 07:04 schemas/commonSchema.js
   221B Dec  1 09:48 schemas/sample.js
   212B Nov 28 07:04 test/common.js
   2.2K Dec  1 09:48 test/ping-centre.js

We could probably not publish the following to npm:

• .eslintrc (1.6K)
• Makefile (63B)
• README.md (3.4K)
• example.js (532B)
• test/common.js (212B)
• test/ping-centre.js (2.2K)

Meh. Save about 8-9K.

Here's the current behavior if I try running ./example.js locally (with a couple minor tweaks):

Input:

// example.js

const topic = process.env.PING_CENTRE_TOPIC;
const PingCentre = require("./src/ping-centre");
const pingClient = new PingCentre(topic, "clientID", "https://onyx_tiles.stage.mozaws.net/v3/links/activity-stream");

pingClient.validate({
  event_type: "search",
  value: 3
});

Output:

$ PING_CENTRE_TOPIC=commonSchema node example

(node:1492) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: Invalid payload {"event_type":"search","value":3}.

$ node --version # v6.9.5
$ npm --version # 3.10.10

Not super helpful failure message, but if I sprinkle some .then().catch() magic, things get a bit better:

Input:

const topic = process.env.PING_CENTRE_TOPIC;
const PingCentre = require("./src/ping-centre");
const pingClient = new PingCentre(topic, "clientID", "https://onyx_tiles.stage.mozaws.net/v3/links/activity-stream");

pingClient.validate({
  event_type: "search",
  value: 3
})
.then(data => console.log(data))
.catch(err => console.error(err));

Output:

$ PING_CENTRE_TOPIC=commonSchema node example

{ Error: Invalid payload {"event_type":"search","value":3}.
    at Joi.validate (/Users/pdehaan/dev/github/mozilla/ping-centre/src/ping-centre.js:28:25)
    at _class._validateWithOptions (/Users/pdehaan/dev/github/mozilla/ping-centre/node_modules/joi-browser/dist/joi-browser.js:5163:21)
    at _class.root.validate (/Users/pdehaan/dev/github/mozilla/ping-centre/node_modules/joi-browser/dist/joi-browser.js:197:24)
    at Promise (/Users/pdehaan/dev/github/mozilla/ping-centre/src/ping-centre.js:26:11)
    at PingCentre.validate (/Users/pdehaan/dev/github/mozilla/ping-centre/src/ping-centre.js:25:12)
    at Object.<anonymous> (/Users/pdehaan/dev/github/mozilla/ping-centre/example2.js:5:12)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
  details: [ '"client_id" is required', '"topic" is required' ],
  payload: { event_type: 'search', value: 3 } }

So it looks like I still need to pass a client_id and topic to .validate(), even though I specified them in the constructor.

Adding those two missing props seems to work and validates as expected:

Input

const topic = process.env.PING_CENTRE_TOPIC;
const PingCentre = require("./src/ping-centre");
const pingClient = new PingCentre(topic, "clientID", "https://onyx_tiles.stage.mozaws.net/v3/links/activity-stream");

pingClient.validate({
  event_type: "search",
  value: 3,
  client_id: 'clientID',
  topic
})
.then(data => console.log(data))
.catch(err => console.error(err));

Output:

$ PING_CENTRE_TOPIC=commonSchema node example

{ event_type: 'search',
  value: 3,
  client_id: 'clientID',
  topic: 'commonSchema' }

SUCCESS! Now that my example ping validates, if I change the .validate() to .sendPing(), it implodes with another error:

Input:

const topic = process.env.PING_CENTRE_TOPIC;
const PingCentre = require("./src/ping-centre");
const pingClient = new PingCentre(topic, "clientID", "https://onyx_tiles.stage.mozaws.net/v3/links/ping-centre");

pingClient.sendPing({
  event_type: "search",
  value: 3,
  client_id: 'clientID',
  topic
})
.then(data => console.log(data))
.catch(err => console.error(err));

Output:

$ PING_CENTRE_TOPIC=commonSchema node example

Ping failure with response code: 400
Error: Ping failure with response code: 400
    at fetch.then.response (/Users/pdehaan/dev/github/mozilla/ping-centre/src/ping-centre.js:53:17)
    at process._tickCallback (internal/process/next_tick.js:103:7)

It'd be nice to see if we're at 100% code coverage or closer to 60%.

There is a field called "raw" in the Test Pilot schema, unfortunately, this is also a reserved keyword for Redshift, see details here.

@6a68 Could you consider renaming this field, please?

For activity stream on iOS, we generate a bad/undesired event whenever we encounter a highlight or top site that doesn't have an image or favicon. Currently ping centre only supports sending pings one at a time causing the panel to make potentially many network calls for each bad event it encounters.

Can we add an API on the service side to accept an array of event data instead of a singular event?

The Snooze Tabs experiment defines its own Redshift schema with custom fields not found in the Test Pilot ping-centre schema, specifically, snooze_time and snooze_time_type.

Could a separate Redshift table be created that matches the documented schema? And, could the existing pings be inserted into that table?

I'm not yet sure if it would also be good to add the raw_ping field to the linked Redshift schema. Some queries might be more useful if they could be sorted by Firefox version. @lmorchard any thoughts? The raw_ping field contains the entire Telemetry ping, serialized as JSON.

Copy of mozilla/activity-stream#2844

Ref: #36 (comment)

Currently in the PingCentre() constructor we "blindly" try require()ing a user-specified topic schema from disk.
We may want to do a bit more validation before loading files, in case user's try and pass funny path strings like "../../../../etc/host" or something (not that it'd work, but still).

Mozilla project guidelines request MPL2 for code-based repositories unless outside constraints apply (e.g. Wordpress plugins and GPL), but any indication of licensing in a top-level LICENSE file is preferable to none.

Since joi isn't really designed to work in the browser (see issue 528 on the joi issue tracker, for example), here are some possible alternatives. They vary quite a bit in terms of power so it depends on what you need.

I did a quick test for build sizes are with webpack, with production optimizations on:

joi-browser           162kb
yup                    77kb
js-schema              16kb
react-schema         5.49kb

The schema is still under discussion, we are likely going to have two topics for "firefox-onboarding", i.e. "firefox-onboarding-sessions" and "firefox-onboarding-events", respectively.

https://mana.mozilla.org/wiki/display/DATAPRACTICES/General+Data+Collection+Guidelines

In general, 180 days is the retention period, and that should be the default.

It looks like ping-centre is using node-config to grab the 'endpoint' config value, based on NODE_ENV.

Unfortunately, the node-config package causes problems with an ES6 build chain, because it uses conditional requires, and those are explicitly disallowed in ES6.

I'm not sure what the best option is here. Maybe another library? Maybe try to get the node-config fix landed?

https://github.com/mozilla/ping-centre/blob/master/ping-centre.js#L33

Here we pass the data into the validator and pass a callback in to handle the validation error case however this does not block the calling scope from moving immediately on to fetch so this will not prevent us from fetching with invalid data. We need to create a Promise and then either reject or resolve on it and then in the then case move on to fetch and resolve the outer promise using the success or failure of the fetch promise.

In the testpilottest.js, both "payload" and "variants" are defined as object fields in the schema. Unfortunately, Redshift doesn't support the nested object in the schema. To work around this limitation, we could consider using following options:

• Investigate the JSON support of Redshift at here
• Introducing the experiment_id to track the A/B test for each Testpilot test, this is already being used by "Activity Stream", and could replace the "variants" field
• Learn from the universal telemetry to handle this kind of payload

Copy of mozilla/activity-stream#2842

So that we can use it for both A-S and other projects

It looks like joi-browser provides a custom build of joi that is ES5-compatible and smaller (excluding the hoek crypto library by default).

I'm not sure if it's worth switching, but might be something to consider.

We've observed that the vast majority of "raw" rows in the "testpilot" table are populated by "0" in the Redshift. The raw logs showed that most of the "raw" fileds are large JSON blobs (in 10KBs). Unfortunately, the ping-centre data processing (Disco) has a hard limit (8KBs) for the key/value size, and the system inappropriately set those overflowed fields to zero values.

This bug has made the "raw" field inaccurate, and will be fixed soon.

On the other hand, I'd recommend to consider shrinking this field for following benefits:

• To save space in Redshift, big JSON blobs will grow the database quickly
• To improve the query performance. Despite the "raw" field isn't deemed to be frequently used, keeping it compact can result in a better query speed
• To limit the scope of data collection, there are a lot of user specif fields in the payload. Including all of them might introduce privacy concern as a result

@6a68 Shall we consider auditing this "raw" field? We'd like to hear your thoughts.

But keep it on by default

We should add eslint as part of the build process. You can find examples here:

https://github.com/mozilla/page-metadata-parser/blob/master/package.json

I noticed that the new test pilot ping sends an array of events rather than 1 at a time. The common schema in this case does not make sense because it expects one event type per ping.

Let's have a second common schema that takes this into account and let's agree that the event type field will have a common name. Right now we've got event_type in the common schema vs. event in the test pilot schema

@6a68 mentioned about there is a built-in utility function navigator.sendBeacon available.

I think it's worth investigating that if it's suitable for Ping-centre. Note that this is a web api other the addon SDK, that means we could use it directly on the content side as well as the addon side through the hidden window.

Currently it looks like if some data fails validation, we abort early and only show the first ValidationError.

That means that we get some errors like the following:

Joi Handles Various Cases
Invalid payload {"topic":"sample","client_id":"47304764-f5b1-4037-81b0-425293963629"}: ValidationError: child "event_type" fails because ["event_type" is required].

Joi#validate() lets you pass an optional options object, which lets you specify abortEarly param which:

abortEarly - when true, stops validation on the first error, otherwise returns all the errors found. Defaults to true.

IIUC, we can pass {abortEarly: false} in PingCentre.validate() to get all validation errors;

Joi Handles Various Cases
Invalid payload {"topic":"sample","client_id":"e0867855-e0ee-4fe2-90f7-29924696f39b"}: ValidationError: child "event_type" fails because ["event_type" is required]. child "value" fails because ["value" is required].

We should include the onyx endpoint in a config file rather than hardcoding it.

Here we list eslint as a dependency, but I think it should be a dev dependency?

https://github.com/mozilla/ping-centre/blob/master/package.json#L23

https://sql.telemetry.mozilla.org/dashboard/activity-stream-system-addon-performance

Copy of mozilla/activity-stream#2838

This serves as the meta bug to track the progress of processing for the Test Pilot pings, which includes following tasks,

• Add table schema in Redshift
• Implement the ETL job in Infernyx

• client_id
• event_type
• topic

Probably using joi (https://github.com/hapijs/joi)

commonSchema.js defines three fields: client_id, topic, and event_type.

client_id and topic are inserted by ping-centre.js, but event_type is not.

Is this a bug or intentional?

• Migrate Telemetry Sender (#63)
• Configurable Endpoint (#64)
• Joi Validation (#65)

I'm guessing node-fetch is unused in package.json, since we're using isometric-fetch.

$ depcheck

Unused dependencies
* node-fetch

Unused devDependencies
* mocha

Missing dependencies
* ping-centre

The mocha and ping-centre warnings are red herrings. I think mocha gets called from a Makefile, and the confusing [self referential] ping-centre reference comes from the example.js file.

When the Firefox SDK loads code, it's loaded into an environment in which there's no window, no self, and no global. This throws off pretty much every fetch polyfill detection approach, which means ping-centre doesn't load properly.

For instance, in isomorphic-fetch, the browser-compatible version uses whatwg-fetch, which in turn assumes that the global object is available as either self or this. In the case of an SDK addon, it attaches its fetch to the wrong object, and the polyfill doesn't work.

So, this is one way to get fetch into the global context in an SDK addon:

require('chrome').Cu.importGlobalProperties(["fetch"]);

I'm working on a PR that tries the SDK approach before loading any fetch libraries. (Another approach would be to have a build step that creates an SDK-compatible ping-centre.js and a web/node compatible ping-centre.js, but I noticed you've got a Makefile in this repo, and I don't totally know how I'd do that with make.) Hopefully will have the PR out within a day or so.

Copy of mozilla/activity-stream#2839

https://github.com/mozilla/ping-centre/blob/master/ping-centre.js#L30-L31

Here we're assigning the data payload to the client instance and resetting, however this can easily cause race conditions where two concurrent callers can overwrite one anothers payloads and lose data easily. The payload only needs to exist for the duration of the send ping call scope.

https://github.com/mozilla/testpilot/blob/master/docs/metrics/telemetry.md#data-collection-details