1515973 - Add Telemetry review guidelines to in-tree docs

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Georg Fritzsche [:gfritzsche]]

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 1

[Georg Fritzsche [:gfritzsche]]

Attachment: Add Telemetry review guidelines to in-tree docs

Comment 2

[Georg Fritzsche [:gfritzsche]]

Attachment: Add Telemetry review guidelines to in-tree docs

Comment 3

[Chris H-C :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?

Comment 4

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Attachment: Add Telemetry review guidelines to in-tree docs

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4991df58d584
Add Telemetry review guidelines to in-tree docs. r=chutten

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/4991df58d584