depend on datazilla client

RESOLVED FIXED

Status

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: k0scist, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
Talos should use https://github.com/mozilla/datazilla_client to post
data to datazilla
(Reporter)

Comment 1

6 years ago
The client require json/simplejson so we will have to resolve bug 744405 prior to this (or concurrently)
Depends on: 744405
(Reporter)

Updated

6 years ago
Blocks: 721097
(Reporter)

Updated

6 years ago
Depends on: 774480
Pythons that we will have to support in mozharness production:
fedora 32, 64: 2.6.2
leopard 2.5.1 (EOL fx 17)
snow 2.6.1
lion 2.7.1

If we only need this for non-leopard in mozharness, we're good targeting 2.6+.
If we need this on leopard, we have to target 2.5.1+.
If we need this for buildbot talos (non-mozharness), we'll need 2.4+.
(Reporter)

Comment 3

6 years ago
So this may actually be impossible until we deprecate talos on python 2.4 or modify datazilla_client.

https://github.com/mozilla/datazilla_client/blob/master/dzclient/__init__.py

uses relative imports, which I believe appear in python 2.5.

Comment 4

6 years ago
The modification there is pretty trivial (removal of exactly one period), so I don't think it should block this. Obviously whenever we can drop 2.4 support we'd want to put the period back, since implicit relative imports are evil ;-)
(Reporter)

Comment 5

6 years ago
(In reply to Jeff Hammel [:jhammel] from comment #3)
> So this may actually be impossible until we deprecate talos on python 2.4 or
> modify datazilla_client.
> 
> https://github.com/mozilla/datazilla_client/blob/master/dzclient/__init__.py
> 
> uses relative imports, which I believe appear in python 2.5.

Actually, scratch that.  Since we only care about https://github.com/mozilla/datazilla_client/blob/master/dzclient/client.py we don't need this file.  That'll teach me to try to think late in the day.  I haven't surveyed client.py for 2.5+isms, but the __init__ issues can at least be ignored
(Reporter)

Comment 6

6 years ago
So one thing we'll have to figure out in production is how we're going to deal with the client key for oauth2.  :jmaher, :jeads, any ideas?  (And please feel free to mark the bug security if there's anything non-public worth saying)

Comment 7

6 years ago
(In reply to Jeff Hammel [:jhammel] from comment #6)
> So one thing we'll have to figure out in production is how we're going to
> deal with the client key for oauth2.  :jmaher, :jeads, any ideas?  (And
> please feel free to mark the bug security if there's anything non-public
> worth saying)

I vaguely recall :ctalbert saying at one point a while back that this could be handled via talos.json.
(Reporter)

Comment 8

6 years ago
(In reply to Carl Meyer (:carljm) from comment #7)
> (In reply to Jeff Hammel [:jhammel] from comment #6)
> > So one thing we'll have to figure out in production is how we're going to
> > deal with the client key for oauth2.  :jmaher, :jeads, any ideas?  (And
> > please feel free to mark the bug security if there's anything non-public
> > worth saying)
> 
> I vaguely recall :ctalbert saying at one point a while back that this could
> be handled via talos.json.

talos.json is public http://hg.mozilla.org/mozilla-central/file/eea94a9b40a1/testing/talos/talos.json so I'm not sure if that works; :ctalbert?
(Reporter)

Updated

6 years ago
Depends on: 776612

Comment 9

6 years ago
I didn't realize talos.json was public. There are tons of areas where the releng automation uses various secret keys. We don't need to re-invent any architecture here. Just download something off one of the releng non-public servers, use it and clean it up. Ask Armenzg or bhearsum for a proper location.
(Reporter)

Comment 10

6 years ago
(In reply to Clint Talbert ( :ctalbert ) from comment #9)
> I didn't realize talos.json was public. There are tons of areas where the
> releng automation uses various secret keys. We don't need to re-invent any
> architecture here. Just download something off one of the releng non-public
> servers, use it and clean it up. Ask Armenzg or bhearsum for a proper
> location.

I ticketed what to do with credentials at bug 776612. I marked it confidential in case any security issues came up. I'm certainly not asking any architecture reinvention...I mostly just need to know how to make talos actually read the credentials, which, since releng will keep and deploy the credentials for production, is really their call
(Reporter)

Comment 11

6 years ago
so datazilla_client uses a minimal constructor like:

req = DatazillaRequest(
    host="datazilla.mozilla.org",
    project="project",
    oauth_key="oauth-key",
    oauth_secret="oauth-secret",
    )

We need the request method even to get the JSON string of results.  I'm not precisely sure what a good architecture is for this for the time
(Reporter)

Comment 12

6 years ago
So this will probably require a few API changes for talos.  Currently we use a datazilla url like:

http://10.8.73.29/views/api/load_test

(See http://hg.mozilla.org/build/talos/file/c583afd8c89a/talos/PerfConfigurator.py#l288)

However, the datazilla_client takes a host and project.  See comment 11
(Reporter)

Comment 13

6 years ago
Created attachment 648136 [details]
example output

example output for the v8 benchmark
(Reporter)

Comment 14

6 years ago
Created attachment 649420 [details] [diff] [review]
WIP

This is sadly bitrotted, but has pretty much everything except posting.  I'll unbitrot and put something for posting, but bug 776612 prevents an end-to-end chain right now
(Reporter)

Comment 15

6 years ago
Created attachment 649469 [details] [diff] [review]
WIP 2

This is about as far as I can get without bug 776612 being resolved.  How much is left to do kinda depends on how we pass in the data.  It may be necessary to restructure how we pass things into results -> output as we currenly only pass the urls (and have a few things in results).

Also, https://bugzilla.mozilla.org/show_bug.cgi?id=731159#c23 . Probably not actually related but worth bearing in mind
Attachment #649420 - Attachment is obsolete: true
Attachment #649469 - Flags: feedback?(jmaher)
(Reporter)

Comment 16

6 years ago
Oh, forgot to mention this depends on the trunk of datazilla_client, not on pypi yet.  Once everything is good to go, pypi will be updated
Comment on attachment 649469 [details] [diff] [review]
WIP 2

Review of attachment 649469 [details] [diff] [review]:
-----------------------------------------------------------------

::: setup.py
@@ +10,5 @@
>  
>  version = "0.0"
>  
> +dependencies = ['PyYAML',
> +                'mozdevice >= 0.2',

can we make mozdevice 0.3 here?  That will save another bug.

::: talos/PerfConfigurator.py
@@ +285,5 @@
>  
>          # default raw_results_url
>          # TODO: deprecate and set via invoker (buildbot, etc)
>          if not self.config.get('datazilla_urls'):
> +            self.config['datazilla_urls'] = ["http://10.8.73.29/views"]

should we switch to the live one now?

::: talos/output.py
@@ +431,5 @@
>      def post(self, results, server, path, scheme):
>  
> +        project = path.strip('/')
> +        req = DatazillaRequest.create(scheme, server, project, None, None, results)
> +        req.submit()

are there any try/excepts we need here?

::: talos/results.py
@@ +408,5 @@
>                      counter_results[counter_name].append(value)
>  
>      def shutdown(self, counter_results):
>          """record shutdown time in counter_results dictionary"""
> +        counter_results.setdefault('shutdown', []).append(int(self.endTime - self.startTime))

int or float?
Attachment #649469 - Flags: feedback?(jmaher) → feedback+
(Reporter)

Comment 18

6 years ago
(In reply to Joel Maher (:jmaher) from comment #17)
> Comment on attachment 649469 [details] [diff] [review]
> WIP 2
> 
> Review of attachment 649469 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: setup.py
> @@ +10,5 @@
> >  
> >  version = "0.0"
> >  
> > +dependencies = ['PyYAML',
> > +                'mozdevice >= 0.2',
> 
> can we make mozdevice 0.3 here?  That will save another bug.

Sure

> ::: talos/PerfConfigurator.py
> @@ +285,5 @@
> >  
> >          # default raw_results_url
> >          # TODO: deprecate and set via invoker (buildbot, etc)
> >          if not self.config.get('datazilla_urls'):
> > +            self.config['datazilla_urls'] = ["http://10.8.73.29/views"]
> 
> should we switch to the live one now?

I don't see why not.

> ::: talos/output.py
> @@ +431,5 @@
> >      def post(self, results, server, path, scheme):
> >  
> > +        project = path.strip('/')
> > +        req = DatazillaRequest.create(scheme, server, project, None, None, results)
> > +        req.submit()
> 
> are there any try/excepts we need here?

That's a good question.  The short answer is:  I don't know.  DatazillaClient should have some sort of error handling here (though it probably mostly doesn't), and Talos should have other pieces.  I don't really know what those are though I'll try to figure these pieces out.  We should probably enumerate the possibilities

> ::: talos/results.py
> @@ +408,5 @@
> >                      counter_results[counter_name].append(value)
> >  
> >      def shutdown(self, counter_results):
> >          """record shutdown time in counter_results dictionary"""
> > +        counter_results.setdefault('shutdown', []).append(int(self.endTime - self.startTime))
> 
> int or float?

Currently, we get a (python) long int out for both self.endTime and self.startTime and subtracting them also gives a long int (even though the value is fairly small).  I cast to an int to prevent the long int nonsense, though it can just as easily be a float
(Reporter)

Comment 19

6 years ago
as specced out in bug 776612, we will use --authfile to specify the credentials in the form of http://hg.mozilla.org/build/buildbot-configs/file/default/mozilla/passwords.py.template for keys oauthSecret, oauthKey
(Reporter)

Comment 20

6 years ago
Created attachment 651508 [details] [diff] [review]
proposed implementation

The actual posting part is untested.  To use this patch you must be using the trunk of datazilla_client, https://github.com/mozilla/datazilla_client/, which is not yet on pypi.
Attachment #649469 - Attachment is obsolete: true
Attachment #651508 - Flags: review?(jmaher)
(Reporter)

Comment 21

6 years ago
Correction, datazilla_client has been released to pypi now :)
Comment on attachment 651508 [details] [diff] [review]
proposed implementation

Review of attachment 651508 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this looks really good.

::: talos/output.py
@@ +4,5 @@
>  
>  """output formats for Talos"""
>  
>  import filter
> +import imp

imp? python v2.4?
Attachment #651508 - Flags: review?(jmaher) → review+
(Reporter)

Comment 23

6 years ago
Yep, imp works on python 2.4 too
(Reporter)

Comment 24

6 years ago
pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=491bcb172689  I wouldn't be at all surprised if there were errors.
(Reporter)

Comment 25

6 years ago
(In reply to Jeff Hammel [:jhammel] from comment #24)
> pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=491bcb172689  I
> wouldn't be at all surprised if there were errors.

So far, the failures are all https://github.com/mozilla/datazilla_client/issues/24

Comment 26

6 years ago
Try run for 491bcb172689 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=491bcb172689
Results (out of 69 total builds):
    exception: 5
    success: 23
    warnings: 1
    failure: 40
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jhammel@mozilla.com-491bcb172689
(Reporter)

Comment 27

6 years ago
So this doesn't deal with error handling at all, part of which should probably happen in datazilla_client and part of which will need to happen in talos.  I don't really know of a good way to bootstrap this, outside of taking :jeads' time to help map out what all can/will go wrong. 

At least we should make some noise when we're sending to a URL in talos.  I'll make this change and hope it doesn't break all the things I'm currently testing on try:

https://tbpl.mozilla.org/?tree=Try&rev=c427d78c08a4

So far, so green.
(Reporter)

Comment 28

6 years ago
And the changesets do make it in to datazilla.  Going to land this.
(Reporter)

Comment 29

6 years ago
pushed: http://hg.mozilla.org/build/talos/rev/e3ab367914e6
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.