Skip to content

Commit 6a609c6

Browse files
authored
🏗 Command line tool to sweep experiments (ampproject#31458)
Sweeps experiments by id, or when they were last flipped before a certain cutoff date. If their removal causes modification of runtime or test code, it requires follow-up intervention. This runs manually, but could potentially be triggered by a scheduled bot to initiate pull requests. ``` gulp sweep-experiments --days_ago: How old experiment configuration flips must be for an experiment to be removed. Default is 365 days. This is ignored when using --experiment. --dry-run: Don't write, but only list the experiments that would be removed by this command. --experiment: Remove a specific experiment id. ``` See README.md for more details.
1 parent 496baf2 commit 6a609c6

File tree

6 files changed

+911
-1
lines changed

6 files changed

+911
-1
lines changed
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,236 @@
1+
# Sweep experiments tool
2+
3+
Sweeps experiments by id, or when they were last flipped before a certain cutoff date.
4+
5+
## Command line
6+
7+
By default, removes experiments whose [production launch value](../../global-configs/prod-config.json) was last set to either `1` or `0` over a year ago:
8+
9+
```sh
10+
gulp sweep-experiments
11+
```
12+
13+
The tool can also sweep a specific experiment by id, regardless of its launch value:
14+
15+
```sh
16+
gulp sweep-experiments --experiment=my-experiment
17+
```
18+
19+
For all available options, see the [exported flags.](./index.js)
20+
21+
## Generated commmit history
22+
23+
The tool adds new commits to the history, so you should make sure that the tool runs on a clean branch.
24+
25+
### Removal commits
26+
27+
Each experiment has its own removal commit, so that it can be reverted or referenced independently.
28+
29+
Their description includes the experiment's id, its evaluated launch value, and
30+
previous launch flip history. The most recent flip commit in the history is also referenced on the subject line:
31+
32+
```
33+
(2019-12-01, aa1bb2c) `my-experiment`: 1
34+
35+
Previous history on prod-config.json:
36+
37+
- aa1bb2c - 2019-12-01T00:00:00-08:00 - Launch my-experiment to 100% of production
38+
- c3dd4ee - 2019-10-01T00:00:00-08:00 - Launch my-experiment to 100% of canary
39+
```
40+
41+
These commits remove the following:
42+
43+
1. **Launch bit** entry on `prod-config.json` and `canary-config.json`.
44+
45+
2. **Listing entry** on `experiments-config.js`.
46+
47+
3. **References in source code**, such as `isExperimentOn()`.
48+
49+
References in source code that were removed or replaced **[require additional modification done manually](#followup).**
50+
51+
### Summary commit
52+
53+
A final, empty commit (no file changes) is created, whose description includes a summary report of all changes. This commit can be referenced for a pull request description, or to find files in which to intervene manually.
54+
55+
## <a id="#followup"></a> Manual follow-up changes
56+
57+
Javascript changes on runtime or test code are **not** ready to be submitted, so they require follow-up changes.
58+
59+
Each removal may be different depending on the implementation that referenced the experiment. The following are steps to take on common cases.
60+
61+
### <a id="followup:isExperimentOn"></a> `isExperimentOn()`
62+
63+
Calls to `isExperimentOn()` are replaced their launch % as a boolean literal. This is likely changed on runtime code.
64+
65+
Users of these calls may now be unnecessary, so they should be intervened manually:
66+
67+
#### Assertions
68+
69+
If an assertion like `userAssert` or `devAssert` evaluates on `true`, it should be removed:
70+
71+
```diff
72+
doStuffWhenExperimentIsOn() {
73+
- userAssert(/* isExperimentOn(win, 'my-experiment') // launched: true */ true);
74+
doStuff();
75+
}
76+
```
77+
78+
If an assertion now evalues on `false`, it's likely part of a larger block that
79+
is now dead code:
80+
81+
```diff
82+
- doStuffWhenExperimentIsOff() {
83+
- devAssert(/* isExperimentOn(win, 'my-experiment') // launched: false */ false);
84+
- doStuff();
85+
- }
86+
```
87+
88+
#### Conditional blocks
89+
90+
Conditions that result on `false` should have their block removed altogether, for example:
91+
92+
```diff
93+
- if (/* isExperimentOn(win, 'my-experiment') // launched: false */ false) {
94+
- doStuff();
95+
- } else {
96+
doStuffOtherwise();
97+
- }
98+
```
99+
100+
If the conditions results on `true`, the block should always be executed:
101+
102+
```diff
103+
- if (/* isExperimentOn(win, 'my-experiment') // launched: true */ true) {
104+
doStuff();
105+
- } else {
106+
- doStuffOtherwise();
107+
- }
108+
```
109+
110+
Watch for the complete evaluated result:
111+
112+
```diff
113+
// Remove entire block when condition evaluates to false
114+
- if (!(/* isExperimentOn(win, 'my-experiment') // launched: true */ true)) {
115+
- return;
116+
- }
117+
- if (
118+
- someOtherValue &&
119+
- (/* isExperimentOn(win, 'my-experiment') // launched: false */ false)
120+
- ) {
121+
- return;
122+
- }
123+
124+
// Inverted, so evalues to `true` and its block should always be executed:
125+
- if (!(/* isExperimentOn(win, 'my-experiment') // launched: false */ false)) {
126+
doSomething();
127+
- }
128+
129+
// Literal boolean operand is redundant, but other operands remain:
130+
if (
131+
- (/* isExperimentOn(win, 'my-experiment') // launched: true */ true) &&
132+
someOtherValue
133+
) {
134+
return;
135+
}
136+
if (
137+
- (/* isExperimentOn(win, 'my-experiment') // launched: false */ false) ||
138+
someOtherValue
139+
) {
140+
return;
141+
}
142+
```
143+
144+
Experiment flag values may be set early and read later, so look for conditionals with backward references:
145+
146+
```diff
147+
constructor() {
148+
- this.isMyComponentExperimentOn_ =
149+
- /* isExperimentOn(win, 'my-experiment') // launched: true */
150+
- true;
151+
}
152+
layoutCallback() {
153+
- if (this.isMyComponentExperimentOn_) {
154+
doStuff();
155+
- }
156+
}
157+
```
158+
159+
### <a id="followup:toggleExperiment"></a> `toggleExperiment()`
160+
161+
Calls to `toggleExperiment()` most likely occur on tests.
162+
163+
If a toggle flip matches the experiment launch value, the call has already been safely removed. As a result, useless blocks may be left over, so they should be removed:
164+
165+
```diff
166+
- // Turn on experiment so tests work.
167+
- beforeEach(() => {});
168+
-
169+
- // Turn off experiment to cleanup.
170+
- afterEach(() => {
171+
- /* toggleExperiment(win, 'my-experiment', false) // launched: true */
172+
- false;
173+
- })
174+
```
175+
176+
Otherwise the the test is likely obsolete and will fail, since it requires a state not possible with the current launch value:
177+
178+
```diff
179+
- it('should fail if experiment is off', () => {
180+
- /* toggleExperiment(win, 'my-experiment', false) // launched: true */
181+
- false;
182+
- shouldFail();
183+
- })
184+
185+
it('returns value', () => {
186+
expect(foo()).to.equal('value returned with experiment on');
187+
- /* toggleExperiment(win, 'my-experiment', false) // launched: true */
188+
- false;
189+
- expect(foo()).to.equal('value returned with experiment off');
190+
});
191+
```
192+
193+
### <a id="followup:html"></a> References on HTML files
194+
195+
The summary commit may list HTML files that contain references to an experiment as a string (`"my-experiment"` or `'my-experiment'`).
196+
197+
These should be handled manually:
198+
199+
- **Turned on via `amp-experiment-opt-in`**
200+
201+
Experiments may be turned using a `<meta>` tag, so it should be removed:
202+
203+
```diff
204+
- <meta name="amp-experiments-opt-in" content="my-experiment">
205+
```
206+
207+
If other experiments are opted-into, only some entries should be removed:
208+
209+
```diff
210+
<meta
211+
name="amp-experiments-opt-in"
212+
- content="some-other-experiment,my-removed-experiment"
213+
+ content="some-other-experiment"
214+
>
215+
```
216+
217+
- **Toggled via `<script>` snippet**
218+
219+
Some HTML files include a `<script>` tag that toggles the experiment. In which case, their execution block should be removed:
220+
221+
```diff
222+
- <script>
223+
- (self.AMP = self.AMP || []).push(function(AMP) {
224+
- AMP.toggleExperiment('my-experiment', true);
225+
- });
226+
- </script>
227+
```
228+
229+
- **Benign references**
230+
231+
Some HTML files reference the experiment name if it's homonymous with an unrelated attribute, like when an experiment is named after a new extension. In this case, **there's nothing to remove:**
232+
233+
```diff
234+
<!-- Experiment is called the same as amp-my-extension: -->
235+
<script custom-element="amp-my-extension" ...></script>
236+
```

0 commit comments

Comments
 (0)