Merge lp:~didrocks/ubiquity/intall-metrics into lp:ubiquity
- intall-metrics
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | 6596 |
Proposed branch: | lp:~didrocks/ubiquity/intall-metrics |
Merge into: | lp:ubiquity |
Diff against target: |
390 lines (+150/-21) 8 files modified
debian/changelog (+4/-0) ubiquity/frontend/debconf_ui.py (+5/-1) ubiquity/frontend/gtk_ui.py (+8/-1) ubiquity/frontend/kde_components/PartAuto.py (+7/-5) ubiquity/frontend/kde_ui.py (+7/-1) ubiquity/frontend/noninteractive.py (+5/-1) ubiquity/plugins/ubi-partman.py (+18/-12) ubiquity/telemetry.py (+96/-0) |
To merge this branch: | bzr merge lp:~didrocks/ubiquity/intall-metrics |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jean-Baptiste Lallement | Approve | ||
Iain Lane | Approve | ||
Review via email:
|
Commit message
Description of the change
Add telemetry function
Always report install metrics for optional reporting later on.
Tested on both Kubuntu and Ubuntu, with:
- side-by-side install
- use biggest free space
- reinstall over existing installation, keeping files
- reinstall and replace existing installation
- full disk used
- lvm
- crypto
- manual
- oem
- automatic installation
It should also work in debconf UI mode, but I'm unsure how to test it.
- 6593. By Jean-Baptiste Lallement
-
[ Sebastien Bacher ]
* gui/gtk/stepPrepare. ui:
- updated the non-free-software text to not include the mention to the
MP3 fluendo codecs since those have been superseeded in the recent
gstreamer version. - 6594. By Jean-Baptiste Lallement
-
[ Sebastien Bacher ]
* Rename the "Preparing to install Ubuntu" installer step to "Updates and
other software" according to the design, thanks Matthew for pointing out
the inconsistency - 6595. By Jean-Baptiste Lallement
-
* debian/control: fixed dependency on sensible-utils

Didier Roche-Tolomelli (didrocks) wrote : | # |
Waiting for your comments on my answer before going further :)

Didier Roche-Tolomelli (didrocks) : | # |

Didier Roche-Tolomelli (didrocks) wrote : | # |
ok, sounds like I can't make it a singleton easily without breaking the MVC semantic in place.
I have changed the global as a class variable and made the time measurement relative + adding a "start time"
Let me know what you think about class singleton vs code consistency and "Steps" naming

Iain Lane (laney) wrote : | # |
some replies to your comments but I had to do them on the old version of the diff
thanks for considering. As commented I think we should discuss what the requirements for timing data are as this seems a bit strange to me. After getting that settled and maybe adding some protection to the file saving I'm OK with this once jibel has tested and given his thumbs up. Cheers!

Didier Roche-Tolomelli (didrocks) wrote : | # |
Done of protecting telemetry. Let's wait on finishing our discussion on what to collect :)

Sebastien Bacher (seb128) wrote : | # |
Ok, I've replied on the "recording the steps" comments, those seem like they could be useful to validate design changes and see their impact on the users

Didier Roche-Tolomelli (didrocks) wrote : | # |
Tested latest rebased version with the GTK installer, all working!

Iain Lane (laney) wrote : | # |
ok from my side, waiting for QA to approve too

Jean-Baptiste Lallement (jibel) wrote : | # |
LGTM too. Thanks!
- 6596. By Jean-Baptiste Lallement
-
* Added telemetry feature to collect installation metrics and optional reporting later on. Thanks didrocks
Preview Diff
1 | === modified file 'debian/changelog' |
2 | --- debian/changelog 2018-03-09 10:43:07 +0000 |
3 | +++ debian/changelog 2018-03-13 11:08:32 +0000 |
4 | @@ -12,6 +12,10 @@ |
5 | other software" according to the design, thanks Matthew for pointing |
6 | out the inconsistency |
7 | |
8 | + [ Didier Roche ] |
9 | + * Collect and dump telemetry info as a log file: (LP: #1755456) |
10 | + Those installation telemetry info are for optional upload later on. |
11 | + |
12 | -- Mathieu Trudel-Lapierre <cyphermox@ubuntu.com> Wed, 28 Feb 2018 14:36:54 -0500 |
13 | |
14 | ubiquity (18.04.3) bionic; urgency=medium |
15 | |
16 | === modified file 'ubiquity/frontend/debconf_ui.py' |
17 | --- ubiquity/frontend/debconf_ui.py 2013-02-19 11:33:07 +0000 |
18 | +++ ubiquity/frontend/debconf_ui.py 2018-03-13 11:08:32 +0000 |
19 | @@ -33,7 +33,7 @@ |
20 | |
21 | import debconf |
22 | |
23 | -from ubiquity import i18n |
24 | +from ubiquity import i18n, telemetry |
25 | from ubiquity.components import install, plugininstall |
26 | from ubiquity.frontend.base import BaseFrontend, Controller |
27 | from ubiquity.plugin import Plugin |
28 | @@ -94,6 +94,9 @@ |
29 | 'cannot continue without them.'), file=sys.stderr) |
30 | sys.exit(1) |
31 | |
32 | + telemetry.get().set_installer_type('DebConf') |
33 | + telemetry.get().add_stage(telemetry.START_INSTALL_STAGE_TAG) |
34 | + |
35 | self.pagesindex = 0 |
36 | self.pageslen = 0 |
37 | self.pages = [] |
38 | @@ -152,6 +155,7 @@ |
39 | "Install failed with exit code %s; see " |
40 | "/var/log/syslog" % ret) |
41 | |
42 | + telemetry.get().done(self.db) |
43 | return 0 |
44 | else: |
45 | return 10 |
46 | |
47 | === modified file 'ubiquity/frontend/gtk_ui.py' |
48 | --- ubiquity/frontend/gtk_ui.py 2017-09-28 21:25:09 +0000 |
49 | +++ ubiquity/frontend/gtk_ui.py 2018-03-13 11:08:32 +0000 |
50 | @@ -54,7 +54,7 @@ |
51 | from ubiquity import gtkwidgets |
52 | |
53 | from ubiquity import ( |
54 | - filteredcommand, gsettings, i18n, validation, misc, osextras) |
55 | + filteredcommand, gsettings, i18n, validation, misc, osextras, telemetry) |
56 | from ubiquity.components import install, plugininstall, partman_commit |
57 | import ubiquity.frontend.base |
58 | from ubiquity.frontend.base import BaseFrontend |
59 | @@ -757,6 +757,8 @@ |
60 | self.debconf_progress_cancellable(False) |
61 | self.refresh() |
62 | |
63 | + telemetry.get().set_installer_type('GTK') |
64 | + |
65 | self.set_current_page(0) |
66 | self.live_installer.show() |
67 | |
68 | @@ -821,6 +823,8 @@ |
69 | # postinstall will exit here by calling Gtk.main_quit in |
70 | # find_next_step. |
71 | |
72 | + telemetry.get().done(self.db) |
73 | + |
74 | self.unlock_environment() |
75 | if self.oem_user_config: |
76 | self.quit_installer() |
77 | @@ -1555,6 +1559,7 @@ |
78 | for i in range(len(self.pages))[index + 1:]: |
79 | self.dot_grid.get_child_at(i, 0).set_fraction(0) |
80 | |
81 | + telemetry.get().add_stage(name) |
82 | syslog.syslog('switched to page %s' % name) |
83 | |
84 | # Callbacks provided to components. |
85 | @@ -1645,6 +1650,8 @@ |
86 | return False |
87 | |
88 | def switch_to_install_interface(self): |
89 | + if not self.installing: |
90 | + telemetry.get().add_stage(telemetry.START_INSTALL_STAGE_TAG) |
91 | self.installing = True |
92 | self.lockdown_environment() |
93 | |
94 | |
95 | === modified file 'ubiquity/frontend/kde_components/PartAuto.py' |
96 | --- ubiquity/frontend/kde_components/PartAuto.py 2016-08-26 07:13:37 +0000 |
97 | +++ ubiquity/frontend/kde_components/PartAuto.py 2018-03-13 11:08:32 +0000 |
98 | @@ -252,18 +252,20 @@ |
99 | disk_id = self.extra_options['use_device'][1][comboText][0] |
100 | disk_id = disk_id.rsplit('/', 1)[1] |
101 | option = self.extra_options['resize'][disk_id][0] |
102 | - return option, '%d B' % self.resizeSize |
103 | + return option, '%d B' % self.resizeSize, 'resize_use_free' |
104 | elif choice == self.useDeviceChoice: |
105 | return (self.extra_options['use_device'][0], |
106 | - str(self.part_auto_disk_box.currentText())) |
107 | + str(self.part_auto_disk_box.currentText()), 'use_device') |
108 | elif choice == self.lvm_choice: |
109 | return (choice, |
110 | - str(self.part_auto_disk_box.currentText())) |
111 | + str(self.part_auto_disk_box.currentText()), 'use_lvm') |
112 | elif choice == self.crypto_choice: |
113 | return (choice, |
114 | - str(self.part_auto_disk_box.currentText())) |
115 | + str(self.part_auto_disk_box.currentText()), 'use_crypto') |
116 | + elif choice == self.manualChoice: |
117 | + return choice, None, 'manual' |
118 | else: |
119 | - return choice, None |
120 | + return choice, None, 'unknown' |
121 | |
122 | def on_disks_combo_changed(self, index): |
123 | for e in self.bar_frames: |
124 | |
125 | === modified file 'ubiquity/frontend/kde_ui.py' |
126 | --- ubiquity/frontend/kde_ui.py 2017-01-09 20:15:33 +0000 |
127 | +++ ubiquity/frontend/kde_ui.py 2018-03-13 11:08:32 +0000 |
128 | @@ -41,7 +41,7 @@ |
129 | sip.setapi("QVariant", 1) |
130 | from PyQt5 import QtCore, QtGui, QtWidgets, uic |
131 | |
132 | -from ubiquity import filteredcommand, i18n, misc |
133 | +from ubiquity import filteredcommand, i18n, misc, telemetry |
134 | from ubiquity.components import partman_commit, install, plugininstall |
135 | import ubiquity.frontend.base |
136 | from ubiquity.frontend.base import BaseFrontend |
137 | @@ -454,6 +454,8 @@ |
138 | self.get_string('ubiquity/install/title')) |
139 | self.refresh() |
140 | |
141 | + telemetry.get().set_installer_type('KDE') |
142 | + |
143 | # Start the interface |
144 | self.set_current_page(0) |
145 | |
146 | @@ -529,6 +531,8 @@ |
147 | self.start_slideshow() |
148 | self.run_main_loop() |
149 | |
150 | + telemetry.get().done(self.db) |
151 | + |
152 | quitText = '<qt>%s</qt>' % self.get_string("finished_label") |
153 | rebootButtonText = self.get_string("reboot_button") |
154 | shutdownButtonText = self.get_string("shutdown_button") |
155 | @@ -1121,6 +1125,7 @@ |
156 | self.ui.content_widget.show() |
157 | self.current_page = newPageID |
158 | name = self.step_name(newPageID) |
159 | + telemetry.get().add_stage(name) |
160 | syslog.syslog('switched to page %s' % name) |
161 | if 'UBIQUITY_GREETER' in os.environ: |
162 | if name == 'language': |
163 | @@ -1291,6 +1296,7 @@ |
164 | elif finished_step == 'ubi-partman': |
165 | # Flush changes to the database so that when the parallel db |
166 | # starts, it does so with the most recent changes. |
167 | + telemetry.get().add_stage(telemetry.START_INSTALL_STAGE_TAG) |
168 | self.stop_debconf() |
169 | self.start_debconf() |
170 | self._show_progress_bar(True) |
171 | |
172 | === modified file 'ubiquity/frontend/noninteractive.py' |
173 | --- ubiquity/frontend/noninteractive.py 2014-06-30 13:09:16 +0000 |
174 | +++ ubiquity/frontend/noninteractive.py 2018-03-13 11:08:32 +0000 |
175 | @@ -32,7 +32,7 @@ |
176 | |
177 | from gi.repository import GLib |
178 | |
179 | -from ubiquity import filteredcommand, i18n, misc |
180 | +from ubiquity import filteredcommand, i18n, misc, telemetry |
181 | from ubiquity.components import install, plugininstall, partman_commit |
182 | import ubiquity.frontend.base |
183 | from ubiquity.frontend.base import BaseFrontend |
184 | @@ -77,6 +77,9 @@ |
185 | file=self.console) |
186 | sys.exit(1) |
187 | |
188 | + telemetry.get().set_installer_type('NonInteractive') |
189 | + telemetry.get().add_stage(telemetry.START_INSTALL_STAGE_TAG) |
190 | + |
191 | for x in self.pages: |
192 | if issubclass(x.filter_class, Plugin): |
193 | ui = x.ui |
194 | @@ -110,6 +113,7 @@ |
195 | if ret == 0: |
196 | self.run_success_cmd() |
197 | print('Installation complete.', file=self.console) |
198 | + telemetry.get().done(self.db) |
199 | if self.get_reboot(): |
200 | misc.execute("reboot") |
201 | if ret != 0: |
202 | |
203 | === modified file 'ubiquity/plugins/ubi-partman.py' |
204 | --- ubiquity/plugins/ubi-partman.py 2016-12-02 13:20:16 +0000 |
205 | +++ ubiquity/plugins/ubi-partman.py 2018-03-13 11:08:32 +0000 |
206 | @@ -25,7 +25,8 @@ |
207 | |
208 | import debconf |
209 | |
210 | -from ubiquity import misc, osextras, parted_server, plugin, validation |
211 | +from ubiquity import (misc, osextras, parted_server, plugin, |
212 | + telemetry, validation) |
213 | from ubiquity.install_misc import archdetect |
214 | |
215 | |
216 | @@ -673,22 +674,24 @@ |
217 | |
218 | def get_autopartition_choice(self): |
219 | if self.reuse_partition.get_active(): |
220 | - return self.extra_options['reuse'][0][0], None |
221 | + return self.extra_options['reuse'][0][0], None, 'reuse_partition' |
222 | |
223 | if self.replace_partition.get_active(): |
224 | - return self.extra_options['replace'][0], None |
225 | + return (self.extra_options['replace'][0], None, |
226 | + 'reinstall_partition') |
227 | |
228 | elif self.custom_partitioning.get_active(): |
229 | - return self.extra_options['manual'], None |
230 | + return self.extra_options['manual'], None, 'manual' |
231 | |
232 | elif self.resize_use_free.get_active(): |
233 | if 'biggest_free' in self.extra_options: |
234 | choice = self.extra_options['biggest_free'][0] |
235 | - return choice, None |
236 | + return choice, None, 'resize_use_free' |
237 | else: |
238 | disk_id = self.get_current_disk_partman_id() |
239 | choice = self.extra_options['resize'][disk_id][0] |
240 | - return choice, '%s B' % self.resizewidget.get_size() |
241 | + return (choice, '%s B' % self.resizewidget.get_size(), |
242 | + 'resize_use_free') |
243 | |
244 | elif self.use_device.get_active(): |
245 | def choose_recipe(): |
246 | @@ -702,13 +705,15 @@ |
247 | |
248 | if not ((want_crypto and have_crypto) or |
249 | (want_lvm and have_lvm)): |
250 | - return self.extra_options['use_device'][0] |
251 | + return self.extra_options['use_device'][0], 'use_device' |
252 | |
253 | if want_crypto: |
254 | - return self.extra_options['some_device_crypto'] |
255 | + return (self.extra_options['some_device_crypto'], |
256 | + 'use_crypto') |
257 | |
258 | if want_lvm: |
259 | - return self.extra_options['some_device_lvm'] |
260 | + return (self.extra_options['some_device_lvm'], |
261 | + 'use_lvm') |
262 | |
263 | # Something went horribly wrong, we should have returned |
264 | # earlier |
265 | @@ -717,9 +722,9 @@ |
266 | i = self.part_auto_select_drive.get_active_iter() |
267 | m = self.part_auto_select_drive.get_model() |
268 | disk = m.get_value(i, 0) |
269 | - choice = choose_recipe() |
270 | + choice, method = choose_recipe() |
271 | # Is the encoding necessary? |
272 | - return choice, misc.utf8(disk, errors='replace') |
273 | + return choice, misc.utf8(disk, errors='replace'), method |
274 | |
275 | else: |
276 | raise AssertionError("Couldn't get autopartition choice") |
277 | @@ -3249,10 +3254,11 @@ |
278 | self.preseed('grub-installer/bootdev', self.ui.get_grub_choice()) |
279 | |
280 | if self.current_question.endswith('automatically_partition'): |
281 | - (autopartition_choice, self.extra_choice) = \ |
282 | + (autopartition_choice, self.extra_choice, method) = \ |
283 | self.ui.get_autopartition_choice() |
284 | self.preseed_as_c(self.current_question, autopartition_choice, |
285 | seen=False) |
286 | + telemetry.get().set_partition_method(method) |
287 | # Don't exit partman yet. |
288 | else: |
289 | self.finish_partitioning = True |
290 | |
291 | === added file 'ubiquity/telemetry.py' |
292 | --- ubiquity/telemetry.py 1970-01-01 00:00:00 +0000 |
293 | +++ ubiquity/telemetry.py 2018-03-13 11:08:32 +0000 |
294 | @@ -0,0 +1,96 @@ |
295 | +# -*- coding: utf-8; Mode: Python; indent-tabs-mode: nil; tab-width: 4 -*- |
296 | + |
297 | +# Copyright (C) 2018 Canonical Ltd. |
298 | +# |
299 | +# Functions useful for the final install.py script and for ubiquity |
300 | +# plugins to use |
301 | +# |
302 | +# This program is free software; you can redistribute it and/or modify |
303 | +# it under the terms of the GNU General Public License as published by |
304 | +# the Free Software Foundation; either version 2 of the License, or |
305 | +# (at your option) any later version. |
306 | +# |
307 | +# This program is distributed in the hope that it will be useful, |
308 | +# but WITHOUT ANY WARRANTY; without even the implied warranty of |
309 | +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
310 | +# GNU General Public License for more details. |
311 | +# |
312 | +# You should have received a copy of the GNU General Public License |
313 | +# along with this program; if not, write to the Free Software |
314 | +# Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA |
315 | + |
316 | + |
317 | +import json |
318 | +import os |
319 | +import stat |
320 | +import syslog |
321 | +import time |
322 | + |
323 | +from ubiquity.misc import raise_privileges |
324 | + |
325 | +START_INSTALL_STAGE_TAG = 'start_install' |
326 | + |
327 | + |
328 | +def get(): |
329 | + """Return a singleton _Telemetry instance.""" |
330 | + if _Telemetry._telemetry is None: |
331 | + _Telemetry._telemetry = _Telemetry() |
332 | + return _Telemetry._telemetry |
333 | + |
334 | + |
335 | +class _Telemetry(): |
336 | + |
337 | + _telemetry = None |
338 | + |
339 | + def __init__(self): |
340 | + self._metrics = {} |
341 | + self._stages_hist = {} |
342 | + self._start_time = time.time() |
343 | + self.add_stage('start') |
344 | + self._dest_path = '/target/var/log/installer/telemetry' |
345 | + try: |
346 | + with open('/cdrom/.disk/info') as f: |
347 | + self._metrics['Media'] = f.readline() |
348 | + except FileNotFoundError: |
349 | + self._metrics['Media'] = 'unknown' |
350 | + |
351 | + def add_stage(self, stage_name): |
352 | + """Record installer stage with current time""" |
353 | + self._stages_hist[int(time.time() - self._start_time)] = stage_name |
354 | + |
355 | + def set_installer_type(self, installer_type): |
356 | + """Record installer type""" |
357 | + self._metrics['Type'] = installer_type |
358 | + |
359 | + def set_partition_method(self, method): |
360 | + """Record anynomized partition method""" |
361 | + self._metrics['PartitionMethod'] = method |
362 | + |
363 | + @raise_privileges |
364 | + def done(self, db): |
365 | + """Close telemetry collection |
366 | + |
367 | + Set as installation done, add additional info and save to |
368 | + destination file""" |
369 | + self.add_stage('done') |
370 | + |
371 | + self._metrics['DownloadUpdates'] = db.get('ubiquity/download_updates') |
372 | + self._metrics['Language'] = db.get('localechooser/languagelist') |
373 | + self._metrics['Minimal'] = db.get('ubiquity/minimal_install') |
374 | + self._metrics['RestrictedAddons'] = db.get('ubiquity/use_nonfree') |
375 | + self._metrics['Stages'] = self._stages_hist |
376 | + |
377 | + target_dir = os.path.dirname(self._dest_path) |
378 | + try: |
379 | + if not os.path.exists(target_dir): |
380 | + os.makedirs(target_dir) |
381 | + with open(self._dest_path, 'w') as f: |
382 | + json.dump(self._metrics, f) |
383 | + os.chmod(self._dest_path, |
384 | + stat.S_IRUSR | stat.S_IWUSR | |
385 | + stat.S_IRGRP | stat.S_IROTH) |
386 | + except OSError as e: |
387 | + syslog.syslog(syslog.LOG_ERR, |
388 | + "Exception while storing telemetry data: " + str(e)) |
389 | + |
390 | +# vim:ai:et:sts=4:tw=80:sw=4: |
Thanks, this is great. I have some comments inline, nothing too major. :)
After fixing those, I don't think that you'd need to re-test the full matrix: if it works for one it should work equally for all the others.
Just for the record, as jibel said on IRC, it would be good to test if this works with preseeding. Even if not, that could be fixed later on though.