Once Bug 1237619 is resolved we can report resource utilization and associated metadata to a server for analysis. The initial implementation will be opt-in and unadvertised until we collect some data and shake out any bugs. Then we can nag people to opt-in. :ekyle has set up a collection server in AWS we can use while we get reporting to telemetry sorted out.

This reports the data we collect for 'resouce_usage.json' to a data collection server. This is opt-in and will only occur if the the BUILD_SYSTEM_TELEMETRY environment variable is set to 1. For now, we take the simplistic approach of reporting data for every build in a synchronous call. This seems fine in my local testing, but if it proves problematic, we can move to asynchronous reporting and/or only report data every n builds. I don't intend to land this until I sort out reporting to telemetry rather than the test server we're currently using, but any feedback you have in the meantime would be appreciated. Review commit: https://reviewboard.mozilla.org/r/31051/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/31051/

Comment on attachment 8708435 [details] MozReview Request: Bug 1239296 - Add telemetry_handler function to mach context r?gps https://reviewboard.mozilla.org/r/31051/#review27885 The way Firefox telemetry works and the way I think we should do this is to write files into a directory and have a process that periodically submits all those files as a batch unit. This is probably something that occurs randomly every 1/N mach commands or something like that. For privacy reasons, we will eventually want to persist submitted data so people can audit what they are sending. We know we'll eventually want to record telemetry things for mach commands that aren't "build." So I find it a bit short-sighted to put this code in a build-specific module/command. Here's what I think we should do. There is a `Context` object that is instantiated when running mach. See `mach.main._run`. I think we should populate a key on this context (see `build/mach_bootstrap.py:bootstrap:populate_context()`) that will hold telemetry data. That's part 1. There is a `pre_dispatch_handler` *hook* in mach that gets called before a command is invoked. We added it to check for some first run foo. Again, see `build/mach_bootstrap.py`. I think we should add a `post_dispatch_handler` *hook* that does stuff after command dispatch. Our implementation of this handler will first persist accumulated metrics data (if any) to a well-defined directory (`os.path.join(context.state_dir, 'telemetry', 'outgoing')` or some such) and second randomly submit all unsubmitted telemetry files. I know this is scope bloat. But I don't think it is horrible. It should save us some work down the line. Feel free to push back if all you are trying to do is unlock some proof-of-concept data submission.

Great suggestions, updated patch will be following shortly in case you want to have another look. I have a meeting with telemetry tomorrow afternoon, hopefully I'll have something up for review soon after that.

Comment on attachment 8708435 [details] MozReview Request: Bug 1239296 - Add telemetry_handler function to mach context r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31051/diff/1-2/

Review commit: https://reviewboard.mozilla.org/r/31271/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/31271/

This adds a post_dispatch_handler hook that will be called after each mach invocation and uses it to submit data to telemetry. Review commit: https://reviewboard.mozilla.org/r/31273/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/31273/

Comment on attachment 8708435 [details] MozReview Request: Bug 1239296 - Add telemetry_handler function to mach context r?gps https://reviewboard.mozilla.org/r/31051/#review28185 ::: build/mach_bootstrap.py:225 (Diff revision 2) > + if not 'BUILD_SYSTEM_TELEMETRY' in os.environ: Nit: use "not in" operator. `if 'BUILD_SYSTEM_TELEMETRY' not in os.environ` ::: build/mach_bootstrap.py:229 (Diff revision 2) > + if not os.path.exists(telemetry_dir): > + os.mkdir(telemetry_dir) This is an anti-pattern. The proper way to do this is: try: os.mkdir(telemetry_dir) except OSError as e: if e.errno != errno.EEXIST: raise ::: build/mach_bootstrap.py:235 (Diff revision 2) > + json.dump(data, f) Let's add a `sort_keys=True` in here so output is deterministic. This will matter for when we write unit tests for this. (Which I won't ask you to do because we don't really have a test harness for mach, sadly.)

Comment on attachment 8709119 [details] MozReview Request: Bug 1239296 - add post_dispatch_handler hook to mach r?gps https://reviewboard.mozilla.org/r/31273/#review28191 ::: build/mach_bootstrap.py:12 (Diff revision 1) > +import requests I don't think `requests` will be importable on a fresh run, since we add it to the search path in this file below. If this works on your machine, it is likely because you have `requests` installed in the system Python site-packages. ::: build/mach_bootstrap.py:300 (Diff revision 1) > + # The user is performing a maintenance command. > + if handler.name in ('bootstrap', 'doctor', 'mach-commands', 'mercurial-setup'): > + return > + > + # We are running in automation. > + if 'MOZ_AUTOMATION' in os.environ or 'TASK_ID' in os.environ: > + return > + > + # The environment is likely a machine invocation. > + if sys.stdin.closed or not sys.stdin.isatty(): > + return You copied this code. Please factor it out into a shared function.

Comment on attachment 8709118 [details] MozReview Request: Bug 1239296 - Use telemetry_handler to store build resource data r?gps https://reviewboard.mozilla.org/r/31271/#review28197 ::: python/mozbuild/mozbuild/mach_commands.py:495 (Diff revision 1) > + telemetry_handler = getattr(self._mach_context, > + 'telemetry_handler', None) > + if telemetry_handler: I /think/ we can assume the telemetry handler is always present since the mach_bootstrap.py used by this command should always have it. Although I'm not 100% sure about this because b2g and comm-central. ::: python/mozbuild/mozbuild/mach_commands.py:498 (Diff revision 1) > + usage = monitor.record_resource_usage() > + telemetry_handler(self._mach_context, usage) This is probably acceptable for now. But we'll eventually likely want to put common metadata in the submitted telemetry packet. That way we can easily filter and discern between various submission types. I consider this feature very alpha right now, so omitting it is fine for the time being.

Comment on attachment 8708435 [details] MozReview Request: Bug 1239296 - Add telemetry_handler function to mach context r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31051/diff/2-3/

Comment on attachment 8709118 [details] MozReview Request: Bug 1239296 - Use telemetry_handler to store build resource data r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31271/diff/1-2/

Comment on attachment 8709119 [details] MozReview Request: Bug 1239296 - add post_dispatch_handler hook to mach r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31273/diff/1-2/

Comment on attachment 8709119 [details] MozReview Request: Bug 1239296 - add post_dispatch_handler hook to mach r?gps https://reviewboard.mozilla.org/r/31273/#review30169 Almost there. ::: build/mach_bootstrap.py:327 (Diff revision 2) > + # Every n-th operation > + if random.randint(1, TELEMETRY_SUBMISSION_FREQUENCY) != 1: > + return Let's move this before the I/O check because it is cheaper. ::: build/mach_bootstrap.py:336 (Diff revision 2) > + if not os.path.exists(submitted): > + os.mkdir(submitted) This is an anti-pattern. This should be: try: os.mkdir(path) except OSError as e: if e.errno != errno.EEXIST: raise ::: build/mach_bootstrap.py:339 (Diff revision 2) > + for filename in os.listdir(outgoing): Should we check for .json extension? ::: build/mach_bootstrap.py:342 (Diff revision 2) > + r = requests.post(BUILD_TELEMETRY_SERVER, data=data, > + headers={'Content-Type': 'application/json'}) I /think/ this will require a new TCP connection for every request. If you create a requests.Session() instance, you can have it reuse the TCP socket, which should make submission faster.

https://reviewboard.mozilla.org/r/31273/#review30169 > Should we check for .json extension? I have a bug in progress to create a json-schema for this data. Validating everything client side might before submission might end up being too slow anyway, but it's worth trying.

Comment on attachment 8708435 [details] MozReview Request: Bug 1239296 - Add telemetry_handler function to mach context r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31051/diff/3-4/

Comment on attachment 8709118 [details] MozReview Request: Bug 1239296 - Use telemetry_handler to store build resource data r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31271/diff/2-3/

Comment on attachment 8709119 [details] MozReview Request: Bug 1239296 - add post_dispatch_handler hook to mach r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31273/diff/2-3/

(In reply to Dan Minor [:dminor] from comment #14) > I have a bug in progress to create a json-schema for this data. Validating > everything client side might before submission might end up being too slow > anyway, but it's worth trying. The server side already verifies the submitted data is JSON, and verifies particular properties have values. If not, it returns a 400. This that enough?

Comment on attachment 8709119 [details] MozReview Request: Bug 1239296 - add post_dispatch_handler hook to mach r?gps https://reviewboard.mozilla.org/r/31273/#review30761 ::: build/mach_bootstrap.py:285 (Diff revision 3) > # We are a curmudgeon who has found this undocumented variable. > if 'I_PREFER_A_SUBOPTIMAL_MERCURIAL_EXPERIENCE' in os.environ: > return > ekr just changed this in a patch I reviewed earlier today. Watch out for bit rot (but the conflict should be trivial to resolve). ::: build/mach_bootstrap.py:320 (Diff revision 3) > + if not 'BUILD_SYSTEM_TELEMETRY' in os.environ: if 'BUILD_SYSTEM_TELEMETRY' not in os.environ ::: build/mach_bootstrap.py:365 (Diff revision 3) > + for filename in os.listdir(submitted): This will iterate directories. Might want to check that `stat.S_ISREG(st.st_mode)` is true.

Comment on attachment 8708435 [details] MozReview Request: Bug 1239296 - Add telemetry_handler function to mach context r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31051/diff/4-5/

Comment on attachment 8709118 [details] MozReview Request: Bug 1239296 - Use telemetry_handler to store build resource data r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31271/diff/3-4/

Comment on attachment 8709119 [details] MozReview Request: Bug 1239296 - add post_dispatch_handler hook to mach r?gps Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31273/diff/3-4/

https://hg.mozilla.org/integration/mozilla-inbound/rev/585954cce5af https://hg.mozilla.org/integration/mozilla-inbound/rev/e4d801abd8dd https://hg.mozilla.org/integration/mozilla-inbound/rev/0f151ac75931

https://hg.mozilla.org/mozilla-central/rev/585954cce5af https://hg.mozilla.org/mozilla-central/rev/e4d801abd8dd https://hg.mozilla.org/mozilla-central/rev/0f151ac75931