Add Telemetry review guidelines to in-tree docs

RESOLVED FIXED in Firefox 66

Status

()

enhancement
P1
normal
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: gfritzsche, Assigned: gfritzsche)

Tracking

Trunk
mozilla66
Points:
---

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Priority: P2 → P1
Attachment #9037988 - Flags: review?(chutten)
Attachment #9037989 - Flags: review?(chutten)
Attachment #9037988 - Attachment is obsolete: true
Attachment #9037988 - Flags: review?(chutten)
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+

(In reply to Chris H-C :chutten from comment #3)

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's a good point.
I'd say we should treat this as a living document where improvements and examples should be added as we notice them.

Attachment #9037989 - Attachment is obsolete: true
Keywords: checkin-needed

Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4991df58d584
Add Telemetry review guidelines to in-tree docs. r=chutten

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
You need to log in before you can comment on or make changes to this bug.