Closed Bug 1449199 Opened 2 years ago Closed Last year

Mitmproxy integration with raptor for OSX

Categories

(Testing :: Raptor, enhancement)

Version 3
enhancement
Not set

Tracking

(firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: rwood, Assigned: rwood)

References

Details

Attachments

(2 files)

So we can playback the tp6 pagesets. Keep as much as this out of mozharness if at all possible. Also keep it as independent as possible, such that the playback tool (mitmproxy/mitmdump) could be swapped out in the future relatively easily.

This bug is for OSX first - will be expanding to other platforms later.
Assignee: nobody → rwood
Status: NEW → ASSIGNED
Hey Andrew,

If you have a chance could you have a look please and provide some feedback? I want to provide a 'generic' playback tool interface. I couldn't figure out how to dynamically inherit a base class i.e. create a 'RaptorPlayback' object and have it inherit a class based on the provided tool name i.e. mitmproxy.

So in this solution I have a 'RaptorPlayback' class that has a 'tool' attribute which creates the playback tool object using the class for the specified tool i.e. mitmproxy.

Maybe there's a better way... ignore all the code in 'mitmproxy.py' except the Mitmproxy class at the bottom for now, all the other code is from our talos mitmproxy implementation and will be worked in.
Attachment #8965826 - Flags: feedback?(ahalberstadt)
Comment on attachment 8965826 [details] [review]
https://github.com/rwood-moz/raptor/pull/24

Sorry, I keep forgetting to set the flag here. Looks good, left some comments/suggestions in github.
Attachment #8965826 - Flags: feedback?(ahalberstadt) → feedback+
Also I missed your comment here, so apologies if some of my comments were on things that you said not to look at
Thanks for the feedback Andrew!
Comment on attachment 8965826 [details] [review]
https://github.com/rwood-moz/raptor/pull/24

Ok, I think I figured out what you meant with abc etc. I believe this is much better :)
Attachment #8965826 - Flags: feedback+ → feedback?(ahalberstadt)
Attachment #8965826 - Flags: feedback?(ahalberstadt) → feedback+
Comment on attachment 8965826 [details] [review]
https://github.com/rwood-moz/raptor/pull/24

Thanks for your help/feedback earlier, appreciated!

I'd like to land this now, just the basic structure for supporting a playback tool. Then I'll build it out later specifically for mitmproxy.
Attachment #8965826 - Flags: feedback+ → review?(ahalberstadt)
Attachment #8965826 - Flags: review?(ahalberstadt) → review+
Merged basic structure for playback tool support:

https://github.com/rwood-moz/raptor/pull/24/commits/0fe6ec8587a6aa0f46758d4b922804a42fa07ed1

Leaving bug open - next is to build it out specifically for mitmproxy.
Next: A patch that integrates mitmproxy into raptor (and changes the name of the raptor 'tp7' test to 'tp6' as it will be replacing the exiting talos tp6 eventually. Also some fixes inside the raptor webext for some bugs I ran into along the way.
Comment on attachment 8970535 [details]
Bug 1449199 - Mitmproxy integration with raptor for OSX;

https://reviewboard.mozilla.org/r/239298/#review244994

a lot of nits, could be unfamiliar with the code- do let me know if there is something I should do instead.

::: .hgignore:186
(Diff revision 2)
>  # Ignore various raptor performance framework files
>  ^testing/raptor/.raptor-venv
>  ^testing/raptor/raptor-venv
>  ^testing/raptor/raptor/tests/.*.json
>  ^testing/raptor/webext/raptor/auto_gen_test_config.js
> +^testing/raptor/raptor/playback/bin

do we download and unpack files here?  I would prefer if we did this in an objdir instead to keep the srcdir clean...although talos does this currently, so maybe a followup?

::: testing/raptor/raptor/control_server.py
(Diff revision 2)
>      def do_GET(self):
>          # get handler, received request for test settings from web ext runner
>          self.send_response(200)
> -        validFiles = ['raptor-firefox-tp7.json',
> +        validFiles = ['raptor-firefox-tp6.json']
> -                      'raptor-chrome-tp7.json',
> -                      'raptor-speedometer.json']

can we keep speedometer?

::: testing/raptor/raptor/playback/mitmproxy.py:24
(Diff revision 2)
> +from mozprocess import ProcessHandler
>  
>  from .base import Playback
>  
>  here = os.path.dirname(os.path.realpath(__file__))
> -tooltool_cache = os.path.join(here, 'tooltoolcache')
> +bin = os.path.join(here, 'bin')

this is a global, could we use gBin or something?

::: testing/raptor/raptor/playback/mitmproxy.py:27
(Diff revision 2)
>  here = os.path.dirname(os.path.realpath(__file__))
> -tooltool_cache = os.path.join(here, 'tooltoolcache')
> +bin = os.path.join(here, 'bin')
> -
>  LOG = get_proxy_logger(component='mitmproxy')
>  
> +TOOLTOOL_URL = 'https://raw.githubusercontent.com/mozilla/build-tooltool/master/tooltool.py'

oh, I don't like a hardcoded path to github

::: testing/raptor/raptor/tests/raptor-firefox-tp6.ini:20
(Diff revision 2)
>  page_cycles = 25
>  
> -[raptor-firefox-tp7]
> -test_url = http://localhost:8081/heroes
> -measure =
> -  fnbpaint
> +[raptor-firefox-tp6]
> +test_url = https://www.amazon.com/s/url=search-alias%3Daps&field-keywords=laptop
> +playback_recordings = mitmproxy-recording-amazon.mp
> +measure = fnbpaint

is this what we use for talos tp6?  I thought it was different, and we have 4 pages for talos tp6.

::: testing/raptor/requirements.txt:4
(Diff revision 2)
>  mozrunner ~= 7.0
>  mozprofile ~= 1.1
>  manifestparser >= 1.1
> +psutil >= 3.1.1

this scares me as we might have other dependencies outside of psutil- could we at least pin this to 3.1.1 (or a specific version)

::: testing/raptor/test/test_playback.py:21
(Diff revision 2)
>      config['playback_tool'] = 'mitmproxy'
> +    config['playback_binary_manifest'] = 'mitmproxy-rel-bin-osx.manifest'
> +    config['playback_binary_zip_mac'] = 'mitmproxy-2.0.2-osx.tar.gz'
> +    config['playback_pageset_manifest'] = 'mitmproxy-playback-set.manifest'
> +    config['playback_pageset_zip_mac'] = 'mitmproxy-recording-set-win10.zip'
> +    config['playback_recordings'] = 'mitmproxy-recording-amazon.mp'

how is recordings different from pageset_zip_mac ?

::: testing/raptor/webext/raptor/measure.js:66
(Diff revision 2)
> -    console.log("will be measuring fnbpaint");
> +      console.log("will be measuring fnbpaint");
> -    measureFNBPaint();
> +      measureFNBPaint();
> -  }
> +    }
> -  if (getFCP) {
> +    if (getFCP) {
> -    console.log("will be measuring first-contentful-paint");
> +      console.log("will be measuring first-contentful-paint");
> -    measureFirstContentfulPaint();
> +      measureFirstContentfulPaint();

can these console.log() and measure*() calls take place above when we set the get*=true?

::: testing/raptor/webext/raptor/runner.js:73
(Diff revision 2)
> -          if (settings.measure.hero.length !== 0) {
> +              if (settings.measure.hero.length !== 0) {
> -            getHero = true;
> +                getHero = true;
> -          }
> +              }
> -        }
> +            }
> +          } else {
> +            console.log("abort: 'measure' key not found in test settings");

will we get two identical console.log() messages from here and measure?
Comment on attachment 8970535 [details]
Bug 1449199 - Mitmproxy integration with raptor for OSX;

Thanks for the review :jmaher!
Attachment #8970535 - Flags: review?(ahalberstadt)
Comment on attachment 8970535 [details]
Bug 1449199 - Mitmproxy integration with raptor for OSX;

https://reviewboard.mozilla.org/r/239298/#review244994

> do we download and unpack files here?  I would prefer if we did this in an objdir instead to keep the srcdir clean...although talos does this currently, so maybe a followup?

Ok thanks, I'll try to change that now. May get tricky as we need the alternate server replay script etc. but should be possible to do via paths. What happens when running against chrome though? Do I make a <path to chrome binary>/testing/raptor dir and put them there?

> can we keep speedometer?

Yeah I'll definitely add it right after, just trying to reduce it down a bit for now. Speedometer will need the benchmark source and small change etc. so was going to add speedometer in a follow-up patch.

> this is a global, could we use gBin or something?

Ok I'll change that thanks

> oh, I don't like a hardcoded path to github

Ok, thanks, I'll copy the tooltool.py into raptor instead.

What I have done here different from talos, is removed mitmproxy from mozharness and put it inside of raptor. Instead of having a mozharness setup-mitmproxy step, raptor takes care of it (and hence this tooltool.py file is required to use tooltool outside of mozharness).

> is this what we use for talos tp6?  I thought it was different, and we have 4 pages for talos tp6.

For talos tp6 we use fnbpaint except for tp6_google we use hero. Yes, I'm just adding mitmproxy support here and needed a single test to pull it al together. Once this lands then I'll need a follow-up patch to actually add the full tp6 test with all 4 pages. :)

> this scares me as we might have other dependencies outside of psutil- could we at least pin this to 3.1.1 (or a specific version)

Ok thanks

> how is recordings different from pageset_zip_mac ?

The pageset_zip_mac is the name of the ZIP file that is downloaded from tooltool, and it contains all the mitmproxy recordings. Recordings is the recording itself i.e. the .mp files that are inside the zip.

> can these console.log() and measure*() calls take place above when we set the get*=true?

Yes good point thanks!

> will we get two identical console.log() messages from here and measure?

No, it won't get that far (the measure functions are in content and only loaded once a new tab is loaded for the first pagecycle, but in this case it doesn't get that far and goes to cleanUp instead).
thanks for answering my questions, I am ok with reduced scope in this patch and expanding in the next bugs.  Regarding the bin location and chrome, I hadn't thought about that.  If we move all the generated/downloaded files into objdir/testing/raptor/..., I could imagine we have objdir/testing/raptor/chrome/chrome.exe or something like that while keeping objdir/dist/bin/firefox.exe for firefox- ideally we just pass a path for the executable binary and we have it already available for firefox, but for chrome it would need to be defined elsewhere.

Maybe we accept the fact that this could change once we add chrome support into our automation- I am not sure if we will be downloading chrome, if we can unpack it and run it from a directory, and what other constraints we will have with the chrome distribution.
Attachment #8970535 - Flags: review?(ahalberstadt)
(In reply to Joel Maher ( :jmaher - limited bugzilla access until May 1st) (UTC+2) from comment #14)

Thanks for all the input, updated patch accordingly. For now I have it using the /objdir/testing/raptor for all mitmproxy downloaded and exe files.
(In reply to Robert Wood [:rwood] from comment #21)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bb4dac263726cef1585776bdbbb6109ccfd7f53

FYI: Just a try push for the in-tree raptor unit tests with this patch applied
Comment on attachment 8970535 [details]
Bug 1449199 - Mitmproxy integration with raptor for OSX;

https://reviewboard.mozilla.org/r/239298/#review245604

a handful of nits- overall this looks good!

::: testing/raptor/raptor/playback/mitmproxy.py:27
(Diff revision 8)
>  LOG = get_proxy_logger(component='mitmproxy')
>  
> +mozharness_dir = os.path.join(here, '../../../mozharness')
> +sys.path.insert(0, mozharness_dir)
> +
> +TOOLTOOL_PATH = os.path.join(mozharness_dir, 'external_tools', 'tooltool.py')

does this exist on try?  I think so, but we should ensure it does.

::: testing/raptor/raptor/playback/mitmproxy.py:76
(Diff revision 8)
> +        if self.config.get("obj_path", None) is not None:
> +            self.bindir = self.config.get("obj_path")
> +        else:
> +            self.bindir = os.path.normpath(os.path.join(self.config['binary'],
> +                                                        '..', '..', '..', '..',
> +                                                        '..', 'testing', 'raptor'))

this needs a comment...what is 5 directories away!

::: testing/raptor/raptor/playback/mitmproxy.py:108
(Diff revision 8)
>      def download(self):
> -        LOG.info("todo: download mitmproxy release binary")
> +        # download mitmproxy binary and pageset using tooltool
> +        # note: tooltool automatically unpacks the files as well
> +        if not os.path.exists(self.bindir):
> +            os.makedirs(self.bindir)
> +        LOG.info("dowmloading mitmproxy binary")

typo in downloading

::: testing/raptor/raptor/playback/mitmproxy.py:114
(Diff revision 8)
> +        _manifest = os.path.join(here, self.config['playback_binary_manifest'])
> +        self._tooltool_fetch(_manifest)
> +        LOG.info("downloading mitmproxy pageset")
> +        _manifest = os.path.join(here, self.config['playback_pageset_manifest'])
> +        self._tooltool_fetch(_manifest)
> +        LOG.info("todo: do mitmdump --version to try it out and dump version")

is this really a todo, will this be seen in all log files?

::: testing/raptor/raptor/playback/mitmproxy.py:129
(Diff revision 8)
> +                                    self.browser_path,
> +                                    str(scripts_path))
>          return
>  
>      def start(self):
>          LOG.info("todo: start mitmproxy playback")

why is this a todo: statement?

::: testing/raptor/raptor/playback/mitmproxy.py:189
(Diff revision 8)
> +        LOG.info("Installing mitmxproxy CA certficate into Firefox")
> +        # browser_path is exe, we want install dir
> +        browser_install = os.path.dirname(browser_path)
> +        # on macosx we need to remove the last folders 'Content/MacOS'
> +        if mozinfo.os == 'mac':
> +            browser_install = browser_install[:-14]

will this be the case in most versions (10.10+) of osx?  the comment helps if not

::: testing/raptor/raptor/playback/mitmproxy.py:213
(Diff revision 8)
> +        LOG.info("recording path:")
> +        LOG.info(mitmproxy_recording_path)
> +        LOG.info("recordings list:")
> +        LOG.info(mitmproxy_recordings_list)
> +        LOG.info("browser path:")
> +        LOG.info(browser_path)

are these printed on separate lines?  would it make sense to do something like:
LOG.info("mitmdump path: %s" % mitmdump_path)

::: testing/raptor/raptor/playback/mitmproxy.py:236
(Diff revision 8)
> +            # mitmproxy needs some DLL's that are a part of Firefox itself, so add to path
> +            env["PATH"] = os.path.dirname(browser_path) + ";" + env["PATH"]
> +        else:
> +            # mac and linux
> +            param2 = param + ' ' + ' '.join(mitmproxy_recordings)
> +            env["PATH"] = os.path.dirname(browser_path)

do we need to append this to the path instead of rewriting the entire path?
Attachment #8970535 - Flags: review?(jmaher) → review+
Comment on attachment 8970535 [details]
Bug 1449199 - Mitmproxy integration with raptor for OSX;

https://reviewboard.mozilla.org/r/239298/#review245604

Thanks for the reviews! Updated accordingly.

> does this exist on try?  I think so, but we should ensure it does.

Good point thanks - yep, I grabbed this code from codecoverage.py, and found it on a try run, so looks safe!

Running command: ['/usr/bin/python2.7', '/builds/worker/workspace/mozharness/external_tools/tooltool.py', '--url', 'https://tooltool.mozilla-releng.net/', 'fetch', '-m', '/builds/worker/workspace/build/tests/config/tooltool-manifests/linux64/ccov.manifest', '-o', '-c', '/builds/worker/tooltool-cache'] in /tmp/tmpsHIxBk
Pushed by rwood@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8ac380929a55
Mitmproxy integration with raptor for OSX; r=jmaher
https://hg.mozilla.org/mozilla-central/rev/8ac380929a55
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.