We have a first pass of our review guidelines here: https://docs.google.com/document/d/1TwN9-7PkZTEP25eugpJbopp1ocRAkN9Q9YLA_zEz0D4/ We need to add them to the in-tree docs. We also want a "validation bug template", that might be a follow-bug though.
Comment on attachment 9037989 [details] [diff] [review] Add Telemetry review guidelines to in-tree docs Review of attachment 9037989 [details] [diff] [review]: ----------------------------------------------------------------- ...Does this need to be added to internals/index.rst with the others? In the future I think I'd like to see this expanded with more examples. For example, saying "Does this change the format of outgoing data in an unhandled way?" is good, but I think adding "(by adding, removing, or changing the type of a non-metric payload field)" would be more instructive. That being said, the audience of this document is mostly the Firefox Telemetry team, so maybe the context can remain assumed. r+wc ::: toolkit/components/telemetry/docs/internals/review.rst @@ +58,5 @@ > + - Does this change contain test coverage? We require test coverage for every new code, changes and bug fixes. > +- Does this need documentation updates? > + - To the in-tree docs? > + - To the firefox-data-docs? > + - To the glean documentation? Could we linkify these? @@ +69,5 @@ > + - `dev-platform <https://lists.mozilla.org/listinfo/dev-platform>`_ (Gecko / platform developers) > + - `mobile-firefox-dev <https://mail.mozilla.org/listinfo/mobile-firefox-dev>`_ (Mobile developers) > + - fx-team (Firefox staff) > + - Do we need to communicate this to other groups? > + - Data engineering, data science, data stewards, …? Is the use of U+2026 intentional? Should it be ASCII "..." instead? (up to you) @@ +98,5 @@ > + - Any code using `new Date()` should get additional scrutiny > + - Code using `new Date()` should be using Policy so it can be mocked > + - Tests using `new Date()` should use specific dates, not the current one > + - How does this impact Build Faster support/Artifact builds/Dynamic builtin scalars or events? Will this be testable by others on artifact builds? > + - We work in the open: Does the change include words that might scare end users? I remembered another footgun: How does this handle client_id resets and users opting out of data collection?
Attachment #9037989 - Flags: review?(chutten) → review+
Attachment #9037989 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.