Support Pioneer pipeline + merging ideas from pioneer-utils by motin · Pull Request #263 · mozilla/shield-studies-addon-utils

motin · 2018-09-10T17:30:59Z

This PR incorporates relevant parts from pioneer-utils v1.0.10 into shield-studies-addon-utils, making the former repository/code obsolete.

A Pioneer study will thus only need to integrate shield-studies-addon-utils and supply studyType: "pioneer" instead of studyType: "shield" in their configuration. To test this code in an existing add-on: npm install --save motin/shield-studies-addon-utils#issues-142-and-128-support-pioneer-pipeline

Note: In the current implementation, Pioneer studies exhibit the same behavior (including life-cycle telemetry) as ordinary Shield studies. The only difference is that all study telemetry payloads are encrypted.

The browser.study.* API is hereby augmented with the browser.study.getDataPermissions() method, returning the current dataPermissions (shield enabled true/false, pioneer enabled true/false). Note: This is a lighter variant of the data permission related features referenced in #177 (comment)

Fixes #142 (Support different data pipelines)
Fixes #128 (Merge ideas from Pioneer Utils)
Fixes mozilla/pioneer-utils#25 (Request to merge / unfork: Shield-Studies-Addon-Utils)
Fixes mozilla/pioneer-utils#13 (Unit tests)
Fixes mozilla/pioneer-utils#7 (Set up CI)
Fixes mozilla/pioneer-utils#40 (Async/await for endStudy and submitEventPing)
Fixes mozilla/pioneer-utils#41 (Add EVENTS.INSTALLED)

mythmon

This seems pretty straight forward to me. I'd like to hear from Rehan as well though.

mythmon · 2018-09-10T18:17:01Z

-        baseUrls: [
-          "https://qsurvey.mozilla.com/s3/Shield-Study-Example-Survey/?reason=ineligible",
-        ],
+const publicApiTests = function(studyType) {


Why is this const publicApiTests = function(...) instead of function publicApiTests(...)?

Arbitrary. Changing to conform to the style of the rest of the script

mythmon · 2018-09-10T18:20:03Z

+});
+
+describe("PUBLIC API `browser.study` (studyType: pioneer)", function() {
+  publicApiTests.bind(this)("pioneer");


I usually see this written as publicApiTests.call(this, "pioneer"). I don't have any reason to prefer one or the other though.

mythmon · 2018-09-10T18:22:20Z


    parameters: []
+
+  - name: getInternalTestingOverrides


Not really relevant to this review, but why is there both a Yaml and JSON version of the schema? That seems like a duplication risk.

Sure, created a tracking issue: #264

The JSON is generated from YAML. We found it faster to iterate (less verbose, multi-line comments) in YAML.

mythmon · 2018-09-10T18:22:38Z

          return studyUtils._internals;
        },
+
+        getInternalTestingOverrides: async function getInternalTestingOverrides() {


The formatting of this seems inconsistent. Why not async getInternalTestingOverrides() {?

similar issue above with getDataPermissions

Originally all function declarations were generated based on the schema YAML in a consistent manner. I believe prettier causes the observed inconsistencies based on the length of the function names.

rehandalal

I did a pass over this. There's a lot to take it but it generally looks fine to me.

rehandalal · 2018-09-10T22:22:26Z

          return studyUtils._internals;
        },
+
+        getInternalTestingOverrides: async function getInternalTestingOverrides() {


similar issue above with getDataPermissions

motin · 2018-11-01T17:41:12Z

Good news - the packets using shield schemas in pioneer telemetry are now being parsed correctly in the pipeline: mozilla-services/mozilla-pipeline-schemas#199 (comment)

motin · 2018-11-14T16:49:38Z

@gregglind This PR is now ready for review and merge. The code has been verified to work in https://github.com/motin/jestr-pioneer-shield-study and the pings have been verified to end up in the correct final destination for Pioneer studies.

…ing stub class for pioneer

…g pioneer-utils as a dependency

…ility in getStudySetup

…eer opt-in add-on (necessary for the test add-on to pass eligibility checks when tested as a pioneer study)

…ly tracking all pings sent using study utils

…lso in the eyes of the schema guard

motin requested review from gregglind, mythmon and rehandalal September 10, 2018 17:31

motin self-assigned this Sep 10, 2018

motin mentioned this pull request Sep 10, 2018

Proposed roadmap towards a unified well documented API and best practices template for study add-on engineers #114

Closed

mythmon reviewed Sep 10, 2018

View reviewed changes

rehandalal reviewed Sep 10, 2018

View reviewed changes

motin mentioned this pull request Sep 11, 2018

Making sure Shield schemas are available for Pioneer telemetry mozilla-services/mozilla-pipeline-schemas#199

Closed

motin force-pushed the release/5.1.0 branch from cf783ac to ed3f17a Compare September 12, 2018 17:14

motin changed the base branch from release/5.1.0 to master September 12, 2018 17:27

motin force-pushed the issues-142-and-128-support-pioneer-pipeline branch from d673ae7 to 99b242c Compare September 12, 2018 17:27

motin force-pushed the issues-142-and-128-support-pioneer-pipeline branch 4 times, most recently from 5d08c1b to d625caf Compare October 28, 2018 22:01

motin mentioned this pull request Nov 2, 2018

Deprecate this repo mozilla/pioneer-utils#44

Merged

motin added 12 commits November 16, 2018 00:40

Added latest version of pioneer-utils as dependency

d16e84b

Made the tests run twice, one for each shieldType (shield, pioneer)

1e553ee

Moved shield-study-type-specifics to separate class, added correspond…

56523d2

…ing stub class for pioneer

Imported relevant parts from pioneer-utils 1.0.10, no longer requirin…

82391f2

…g pioneer-utils as a dependency

Added browser.study.getDataPermissions() and using it to check eligib…

9969751

…ility in getStudySetup

Synced small-study and test-addon boilerplate code

5d1dad6

Scripts to import/build and selenium instructions to install the pion…

8ec4325

…eer opt-in add-on (necessary for the test add-on to pass eligibility checks when tested as a pioneer study)

Added studySetup.telemetry.internalTelemetryArchive flag for internal…

69a221c

…ly tracking all pings sent using study utils

Removed empty placeholders for tests that are already implemented

fc5d830

Implemented tests for browser.study.getDataPermissions()

1828973

Internal telemetry archive stores ping id and is exposed to the tests

ae20821

Telemetry-related tests simplified and restored

dd7190e

motin added 16 commits November 16, 2018 00:40

Increased wait time for expiration test due to intermittent failures

ee44340

Minor cleanup

79910c6

Moved bin/import-pioneer-opt-in.sh to pretest where it belongs

f9046d5

Restored tests

4343075

Fixed a race condition with the expiration-related tests

798790e

Made internalTelemetryArchive nullable so that it is truly optional a…

693db4e

…lso in the eyes of the schema guard

Importing and building the Pioneer opt-in add-on in circle

8d2c880

Nits code style

aa1fded

Correct schema versions specified when sending pioneer telemetry

0d5ee6a

Set import pioneer opt in script as binary in npm package

ee9bddb

Pioneer telemetry logged at debug level like other telemetry log events

32229ab

Commented out a rogue console.log statement

11b28e3

Make eslint happy

162d06c

Add browser.study.calculateTelemetryPingSize

f28bf05

Test for browser.study.calculateTelemetryPingSize

d91775c

Clarified the requirements of the telemetry payload argument

e367173

motin force-pushed the issues-142-and-128-support-pioneer-pipeline branch from 704910d to e367173 Compare November 15, 2018 22:41

motin changed the base branch from master to release/5.2.0 November 15, 2018 22:59

motin mentioned this pull request Nov 27, 2018

Add browser.study.searchSeenTelemetry #276

Open

gregglind merged commit 39f7588 into mozilla:release/5.2.0 Nov 28, 2018

gregglind mentioned this pull request Nov 28, 2018

Revert "Support Pioneer pipeline + merging ideas from pioneer-utils" #277

Closed

motin deleted the issues-142-and-128-support-pioneer-pipeline branch December 2, 2018 20:10

motin mentioned this pull request Dec 2, 2018

Issues 142 and 128 support pioneer pipeline #279

Merged

Conversation

motin commented Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mythmon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rehandalal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

motin commented Nov 1, 2018

Uh oh!

motin commented Nov 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

motin commented Sep 10, 2018 •

edited

Loading