1239296 - Report resource_usage.json to a server

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Dan Minor [:dminor]]

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.

Comment 1

[Dan Minor [:dminor]]

Attachment: MozReview Request: Bug 1239296 - Add telemetry_handler function to mach context r?gps

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 2

[Gregory Szorc [:gps]]

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.

Comment 3

[Dan Minor [:dminor]]

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 4

[Dan Minor [:dminor]]

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/

Comment 5

[Dan Minor [:dminor]]

Attachment: MozReview Request: Bug 1239296 - Use telemetry_handler to store build resource data r?gps

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

Comment 6

[Dan Minor [:dminor]]

Attachment: MozReview Request: Bug 1239296 - add post_dispatch_handler hook to mach r?gps

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 7

[Gregory Szorc [:gps]]

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 8

[Gregory Szorc [:gps]]

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 9

[Gregory Szorc [:gps]]

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 10

[Dan Minor [:dminor]]

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 11

[Dan Minor [:dminor]]

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 12

[Dan Minor [:dminor]]

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 13

[Gregory Szorc [:gps]]

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.

Comment 14

[Dan Minor [:dminor]]

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 15

[Dan Minor [:dminor]]

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 16

[Dan Minor [:dminor]]

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 17

[Dan Minor [:dminor]]

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/

Comment 18

[Kyle Lahnakoski [:ekyle]]

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

[Gregory Szorc [:gps]]

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 20

[Dan Minor [:dminor]]

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 21

[Dan Minor [:dminor]]

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 22

[Dan Minor [:dminor]]

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/

Comment 23

[Pulsebot]

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

Comment 24

[Carsten Book [:Tomcat]]

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