Skip to content

Add regrets-reporter schemas #556

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Jul 21, 2020
Merged

Add regrets-reporter schemas #556

merged 7 commits into from
Jul 21, 2020

Conversation

@motin
Copy link
Contributor

@motin motin commented Jun 4, 2020

... under the telemetry and regrets-reporter namespaces (one for browser.telemetry.submitPing and one for a custom telemetry client)

@auto-assign auto-assign bot requested a review from mreid-moz June 4, 2020 14:30
@motin motin requested a review from acmiyaguchi June 5, 2020 10:58
acmiyaguchi
acmiyaguchi suggested changes Jun 5, 2020
View reviewed changes
Copy link
Contributor

@acmiyaguchi acmiyaguchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than changing the version of the document in the telemetry namespace, this looks good to me.

@motin motin requested a review from acmiyaguchi June 6, 2020 06:39
@motin motin force-pushed the regrets-reporter branch from 8337200 to b5a5b47 Compare June 6, 2020 06:52
@mreid-moz
Copy link
Contributor

mreid-moz commented Jun 8, 2020

@motin can you link to the corresponding Data Collection Review bug?

@motin
Copy link
Contributor Author

motin commented Jun 8, 2020

@motin can you link to the corresponding Data Collection Review bug?

https://bugzilla.mozilla.org/show_bug.cgi?id=1644107#c2

acmiyaguchi
acmiyaguchi approved these changes Jun 10, 2020
View reviewed changes
Copy link
Contributor

@acmiyaguchi acmiyaguchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The schema looks good to me. @mreid-moz is the linked data collection review bug sufficient for r+?

@whd
Copy link
Member

whd commented Jun 11, 2020

Definitely do not merge this. I don't have access to the data review bug (perhaps telling) but given @mreid-moz's recent questions, this data (if we do end up collecting it) may require special provisioning logic that I will need to prepare before this PR is merged.

@acmiyaguchi acmiyaguchi self-requested a review June 11, 2020 00:08
acmiyaguchi
acmiyaguchi suggested changes Jun 11, 2020
View reviewed changes
Copy link
Contributor

@acmiyaguchi acmiyaguchi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing approval to prevent accidental merging.

@motin
Copy link
Contributor Author

motin commented Jul 2, 2020 •
edited
Loading

I don't have access to the CI logs, so I don't know why the tests are failing. Running scripts/mps-build, then scripts/mps-test results in passing tests locally:

$ scripts/mps-test
================================================ test session starts =================================================
platform linux -- Python 3.6.8, pytest-5.3.5, py-1.8.1, pluggy-0.13.1
rootdir: /app
plugins: xdist-1.31.0, forked-1.1.3
collected 374 items

tests/test_validation_java.py ssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssss [ 21%]
sssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssssss    [ 50%]
tests/test_validation_python.py .............................
................................................. [ 70%]
.............................................................................................................  [100%]

==================================== 187 passed, 187 skipped in 206.02s (0:03:26) ====================================

EDIT: After rebasing on current master, the CI tests pass.

@motin motin force-pushed the regrets-reporter branch from 0079f1f to 61c027f Compare July 2, 2020 19:48
@acmiyaguchi
Copy link
Contributor

acmiyaguchi commented Jul 2, 2020

The failure was coming from the BigQuery diff script (docker run -w /app mps pytest scripts/bigquery_schema_diff.py), but it looks like you might have resolved this now.

@motin
Copy link
Contributor Author

motin commented Jul 7, 2020

@mreid-moz @acmiyaguchi Data review is now r+ https://bugzilla.mozilla.org/show_bug.cgi?id=1644107#c17

@motin motin requested a review from acmiyaguchi July 7, 2020 07:19
@whd
Copy link
Member

whd commented Jul 7, 2020

This is still blocked on some policy decisions mapping categories in https://wiki.mozilla.org/Firefox/Data_Collection#Data_Collection_Categories to actual pipeline configuration, which I've filed https://bugzilla.mozilla.org/show_bug.cgi?id=1651005 to address.

@mreid-moz
Copy link
Contributor

mreid-moz commented Jul 20, 2020

This has been approved for collection during the internal staff testing period only. As such we are unblocked to merge this PR. I have filed Bug 1654078 to capture the work that must happen before collecting data any more broadly.

Re-r? @acmiyaguchi to make sure the schema itself looks good.

@mreid-moz mreid-moz requested review from acmiyaguchi and removed request for acmiyaguchi July 20, 2020 20:38
@motin motin requested a review from acmiyaguchi July 21, 2020 14:41
@acmiyaguchi acmiyaguchi merged commit 8b05ee6 into mozilla-services:master Jul 21, 2020
dataops-pipeline-schemas added a commit that referenced this pull request Jul 22, 2020
@dataops-pipeline-schemas
Auto-push from schema generation [ci skip]
e94d330 
8b05ee6	2020-07-21 14:27:14 -0700	Add regrets-reporter schemas (#556)
acmiyaguchi pushed a commit that referenced this pull request Jul 22, 2020
@dataops-pipeline-schemas
Auto-push from schema generation [ci skip]
4be742d 
361c16d	2020-07-22 20:48:09 +0000	Add xfocsp-error-report schema (#581)
8b05ee6	2020-07-21 14:27:14 -0700	Add regrets-reporter schemas (#556)
e724d79	2020-07-17 11:02:54 -0400	Bug 1652842 - Add a blocklist field so the environment schema works with pre- and post-bug-1647225 data (#576)
7400dc8	2020-07-15 12:46:09 +0200	Bug 1652834 - Add gfx.EmbeddedInFirefoxReality field to environment.
490cfe0	2020-07-14 16:39:13 -0400	AET update: 64-char user_ids and relaxing validation
8f22589	2020-07-14 11:56:08 -0400	Specify yaml loader in extract_crash_annotation_fields script (#573)
f0f50c4	2020-07-14 11:55:40 -0400	Bug 1652777 - Add experimental features to crash ping schema (#574)
97bac7a	2020-06-30 15:16:36 -0700	Add 'openglCompositing' and 'wrCompositor' to schema (#569)
c47313b	2020-06-29 18:40:32 +0000	Remove ecosystem_device_id fields (#567)
2cfd30a	2020-06-26 15:09:52 -0400	Make ecosystem_device_id an optional field
3f11d8b	2020-06-23 18:03:10 +0200	Update sync ping schema to make all migration fields transpiled into BQ
e678002	2020-06-22 14:12:25 -0400	add history_average_days_per_month to schema
be61e13	2020-06-22 14:12:25 -0400	fix schema to contain all fields
6ecd201	2020-06-22 14:12:25 -0400	add schema for normandy-login-study bug 1643383
2e62aac	2020-06-22 12:53:32 -0400	Pin Centos image
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mreid-moz mreid-moz Awaiting requested review from mreid-moz

1 more reviewer

@acmiyaguchi acmiyaguchi acmiyaguchi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@motin @mreid-moz @whd @acmiyaguchi