1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
|
===========================
Telemetry review guidelines
===========================
General guidelines for reviewing changes
========================================
These are the general principles we follow when reviewing changes.
- *Be constructive.* Both reviewers and patch authors should be allies that aim to get the change landed together.
- *Consider the impact.* We deliver critical data that is processed and used by many systems and people. Any disruption should be planned and intentional.
- *Be diligent.* All changes should be tested under typical conditions.
- *Know your limits.* Defer to other peers or experts where sensible.
Main considerations for any change
========================================
For any change, these are the fundamental questions that we always need satisfactory answers for.
- Does this have a plan?
- Is there a specific need to do this?
- Does this change need `a proposal <https://github.com/mozilla/Fx-Data-Planning/blob/master/process/ProposalProcess.md>`_ first?
- Do we need to announce this before we do this? (e.g. for `deprecations <https://github.com/mozilla/Fx-Data-Planning/blob/master/process/Deprecation.md>`_)
- Does this involve the right people?
- Does this change need input from... A data engineer? A data scientist?
- Does this change need data review?
- Is this change complete?
- Does this change have sufficient test coverage?
- Does this change have sufficient documentation?
- Do we need follow-ups?
- Do we need to file validation bugs? Or other follow-up bugs?
- Do we need to communicate this to our users? Or other groups?
Additional considerations
=========================
Besides the basic considerations above, these are additional detailed considerations that help with reviewing changes.
Considerations for all changes
------------------------------
- Follow our standards and best practices.
- Firefox Desktop:
- :ref:`The Mozilla coding style <Coding style>`
- `The toolkit code review guidelines <https://wiki.mozilla.org/Toolkit/Code_Review>`_
- Mobile:
- `Android/Kotlin code style <https://kotlinlang.org/docs/reference/coding-conventions.html>`_
- `iOS/Swift code style <https://github.com/mozilla-mobile/firefox-ios/wiki/Swift-Style-Guides>`_
- Does this impact performance significantly?:
- Don't delay application initialisation (unless absolutely necessary).
- Don't ever block on network requests.
- Make longer tasks async whenever feasible.
- Does this affect products more broadly than expected?
- Consider all our platforms: Windows, Mac, Linux, Android.
- Consider all our products: Firefox, Fennec, GeckoView, Glean.
- Does this fall afoul of common architectural failures?
- Prefer APIs that take non-String types unless writing a parser.
- Sanity checking:
- How does this behave in a release build? Have you tested this?
- 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 :ref:`in-tree docs <Telemetry>`?
- To the `firefox-data-docs <https://docs.telemetry.mozilla.org/>`_ (`repository <https://github.com/mozilla/firefox-data-docs>`_)?
- To the `glean documentation <https://github.com/mozilla-mobile/android-components/tree/master/components/service/glean>`_?
- Following up:
- Do we have a validation bug filed yet?
- Do all TODOs have follow-up bugs filed?
- Do we need to communicate this to our users?
- `fx-data-dev <https://mail.mozilla.org/listinfo/fx-data-dev>`_ (Main Firefox data list)
- `firefox-dev <https://mail.mozilla.org/listinfo/firefox-dev>`_ (Firefox application developers)
- `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, ...?
Consider the impact on others
-----------------------------
- Could this change break upstream pipeline jobs?
- Does this change the format of outgoing data in an unhandled way?
- E.g. by adding, removing, or changing the type of a non-metric payload field.
- Does this require ping schema updates?
- Does this break jobs that parse the registry files for metrics? (Scalars.yaml, metrics.yaml, etc.)
- Do we need to involve others?
- Changes to data formats, ping contents, ping semantics etc. require involving a data engineer.
- Changes to any outgoing data that is in active use require involving the stakeholders (e.g. data scientists).
Considerations for Firefox Desktop
----------------------------------
- For profiles:
- How does using different profiles affect this?
- How does switching between profiles affect this?
- What happens when users switch between different channels?
- Footguns:
- Does this have side-effects on Fennec? (Unified Telemetry is off there, so behavior is pretty different.)
- Is your code gated on prefs, build info, channels? Tests should cover that.
- If test is gated on isUnified, code should be too (and vice-versa)
- Test for the other case
- 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?
- How does this handle client id resets?
- How does this handle users opting out of data collection?
|
