Closed Bug 675117 Opened 13 years ago Closed 13 years ago

Use the AMO Perf API instead of database calls

Categories

(Testing :: Talos, defect)

Product:
Testing
Component:
Talos
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: clouserw, Assigned: jmaher)

References

Details

Attachments

(2 files, 3 obsolete files)

cacerts.txt
32.85 KB, text/plain
Details
patch to upload amo results using the amo api (1.1)
22.11 KB, patch
Details | Diff | Splinter Review 
An API was built for these performance numbers in bug 630794.  There is discussion of how to use it from comment 15 on.  I'm filing this bug to track progress as the other bug is closed.
Can you provide a doc of the expected inputs and acceptable values?
Is bug 630794 comment 18 not what you are looking for?
That covers the names of the keys, but I could use some example data (especially for os version since that is changing from the currently understood values).
Those are foreign keys to two tables. OS Version is:

1 	MacOSX 	10.5.8 	
2 	Fedora 	12 - Constantine
3 	WINNT 	5.1 	
4 	WINNT 	6.1

App Version is:

..
21 	fx 	3.6.15pre
22 	fx 	4.0b13pre
23 	fx 	3.6.15
24 	fx 	4.0	
25 	fx 	4.0.1

I don't know where that data came from or what the currently understood thing is however :(
The data comes from Alice's scripts.  Those tables were empty before she started running jobs for us.
Depends on: 675240
What I need a clear idea of here is essentially, how to use this api:

- what are the available api commands?
- what are the acceptable inputs?  what checks are in place to reject bad inputs?
- what indicates a success addition of data?

From comment #4 is looks like it is using the talos concept of os version, but from the other bug it looked like you wanted to adopt a different understand of os version that was more in line with the rest of amo.
Blocks: 690595
Assignee: anodelman → jmaher
this patch is a big WIP and sort of a hack, but I think a step in the right direction to fix this.  Basically I add a amo_api.py script (and the scripts from https://github.com/andymckay/amo-oauth/tree/master/src).  If somebody from the AMO team can get the uploadAMOResults() working, I can test this and get it into production.
Blocks: 695059
I suspect bug 693209 can be resolved if we do the translation in the amo_api.py script.
Blocks: 693209
I obtained a key/secret and I am not able to upload results:
DEBUG: in uploadAMOResults: {'test': 'ts', 'average': '485.75', 'osversion': '3.14', 'appversion': '10.0a1', 'addon': '129535'}
Traceback (most recent call last):
  File "run_tests.py", line 602, in <module>
    test_file(arg, screen, amo)
  File "run_tests.py", line 572, in test_file
    links = send_to_graph(results_server, results_link, title, date, browser_config, results, amo)
  File "run_tests.py", line 274, in send_to_graph
    uploadAMOResults(browser_config['addon_id'], browser_config['browser_version'], testname, val_list)
  File "amo/amo_api.py", line 56, in uploadAMOResults
    print amo.create_perf(data)
  File "amo/amo.py", line 229, in create_perf
    return self._send(self.url('perf'), 'POST', data)
  File "amo/amo.py", line 204, in _send
    data=data)
  File "amo/amo.py", line 113, in _request
    client = httplib2.Http()
  File "amo/amo.py", line 43, in hack
    return old(self, **kw)
TypeError: __init__() got an unexpected keyword argument 'disable_ssl_certificate_validation'


Can somebody take the patch I put on this bug and make it work with the amo api?
I tried the function and it works fine.  You're using addons.allizom.org though and you should be using addons-dev.allizom.org:

In [1]: from amo import AMOOAuth

In [2]: amo = AMOOAuth(domain="addons-dev.allizom.org", port=443, protocol='https', prefix='/z')

In [3]: amo.set_consumer(consumer_key='xxxx',consumer_secret='xxxx')

In [4]: data = {'addon': 2292, 'average': 0.1, 'appversion': 1, 'osversion': 1, 'test': 'ts'}

In [5]: print amo.create_perf(data)
------> print(amo.create_perf(data))
{u'average': 0.10000000000000001, u'appversion': {u'app': u'', u'version': u'browser_version', u'id': 1}, u'test': u'ts', u'osversion': {u'version': u'10.5.8', u'os': u'MacOSX', u'name': u'Mac OS X 10.5.8', u'id': 1}, u'id': 8455, u'addon': {u'status': 4, u'eula': None, u'name': u'On.net toolbar', u'slug': u'onnet-toolbar', u'guid': u'{c10d30ee-36c9-4bda-9b65-c388b2d20f77}', u'id': 2292, u'resource_uri': u'https://addons-dev.allizom.org/z/en-US/firefox/addon/onnet-toolbar/'}}


And to verify from the db:

mysql> select * from perf_results order by id desc limit 1\G
*************************** 1. row ***************************
           id: 8455
     addon_id: 2292
appversion_id: 1
 osversion_id: 1
      average: 0.1
         test: ts
      created: 2011-11-02 09:35:41
     modified: 2011-11-02 09:35:41
1 row in set (0.00 sec)

Note that we just imported the -dev database again and your consumer key/secret had to change.  Talk to me on IRC and I'll give you the new ones
this patch is updated for the latest changes in talos.  When I run this I get:

DEBUG: in uploadAMOResults: {'test': 'ts', 'average': '527.0', 'osversion': '3.14', 'appversion': '11.0a1', 'addon': 'addon.xpi'}
Traceback (most recent call last):
  File "run_tests.py", line 606, in <module>
    main()
  File "run_tests.py", line 603, in main
    test_file(arg, options.screen, options.amo)
  File "run_tests.py", line 563, in test_file
    links = send_to_graph(results_server, results_link, title, date, browser_config, results, amo)
  File "run_tests.py", line 265, in send_to_graph
    uploadAMOResults(browser_config['addon_id'], browser_config['browser_version'], testname, val_list)
  File "amo/amo_api.py", line 33, in uploadAMOResults
    return amo.create_perf(data)
  File "amo/amo.py", line 229, in create_perf
    return self._send(self.url('perf'), 'POST', data)
  File "amo/amo.py", line 206, in _send
    raise ValueError('%s: %s' % (resp.status, content))
ValueError: 400: Bad Request: Invalid data provided: Select a valid choice. That choice is not one of the available choices. (osversion),Select a valid choice. That choice is not one of the available choices. (appversion),Select a valid choice. That choice is not one of the available choices. (addon)


I need somebody to modify this patch and resolve the os version (or tell me what value to put in there if we don't care about it).  

I am also using the key/secret from ~/.amo-auth which seems to be working well.  I am not sure how that will work on windows, but I can test that once we get results that can be uploaded.
Attachment #569104 - Attachment is obsolete: true
Bug 703419 is relevant to this bug!  I met with jmaher earlier today and we hashed out a simple method to get this project done.  Details are in that bug, but I think we're on a roll.
Alright, the simplifcation bug has landed.  I changed your attachment above to submit to '/api/2/performance/add' and then reran with the new data format:


In [1]: from amo import AMOOAuth

In [2]: amo = AMOOAuth(domain="addons-dev.allizom.org", port=443, protocol='https', prefix='/z')

In [3]: amo.set_consumer(consumer_key='xxx',consumer_secret='xxx')

In [4]: data = {'addon_id': 2292, 'average': 1.1, 'os': 'WINNT', 'version': '6.1', 'platform': 'x86', 'product': 'firefox', 'product_version': '8.0a1', 'test': 'ts'}

In [5]: print amo.create_perf(data)
------> print(amo.create_perf(data))
OK

In [6]: print amo.create_perf(data)
------> print(amo.create_perf(data))
OK

Line 6 is proof sending the same thing again is handled gracefully on our end.  You should be able to eliminate the perf_os and perf_app URLs from your script along with any logic using them.  The new format we agreed on is a lot more straightforward and is just a single request from you to us.  Thanks for the help, let us know if there are questions.
patch that meets the needs of using the new api as well as the proper os, version, platform information.

The only thing to resolve is the osversion for linux, right now this is hardcoded to 12.
Attachment #575159 - Attachment is obsolete: true
Attachment #577217 - Flags: review?(wlachance)
Attachment #577217 - Flags: feedback?(clouserw)
Comment on attachment 577217 [details] [diff] [review]
patch to upload amo results using the amo api (1.0)

Ship it! :)

Let's try it on addons-dev.allizom.org.  The credentials you have will work for that.  I can do spot checks in the db for the values anytime, just let me know.  Thanks.
Attachment #577217 - Flags: feedback?(clouserw) → feedback+
Comment on attachment 577217 [details] [diff] [review]
patch to upload amo results using the amo api (1.0)

It might be good to get some more feedback on the addon API parts from people more familiar with that code than me.

In general I think the ideas behind the patch are fine, though I'd like to see a few refinements before this goes in.

># HG changeset patch
># Parent fb920d618a9fb24985c1bebd67ab492aa2462dc2
>
>diff --git a/talos/amo/amo.py b/talos/amo/amo.py
>new file mode 100644
>--- /dev/null
>+++ b/talos/amo/amo.py
>+old = httplib2.Http.__init__
>+
>+
>+# Ouch, I'll go to hell for this.
>+def hack(self, **kw):
>+    return old(self, **kw)
>+
>+httplib2.Http.__init__ = hack

Hmm, I really don't understand this hack. Could we put in a comment to explain?

AFAICT most of this file is just copypasted from elsewhere? In that case probably not worth extensively reviewing...

>diff --git a/talos/amo/amo_api.py b/talos/amo/amo_api.py
>new file mode 100644
>--- /dev/null
>+++ b/talos/amo/amo_api.py
>@@ -0,0 +1,34 @@
>+
>+from amo import AMOOAuth
>+
>+def avg_excluding_max(val_list):
>+  """return float rounded to two decimal places, converted to string
>+     calculates the average value in the list exluding the max value"""
>+  i = len(val_list)
>+  total = sum(float(v) for v in val_list)
>+  maxval = max(float(v) for v in val_list)
>+  if total > maxval:
>+    avg = str(round((total - maxval)/(i-1), 2))
>+  else:
>+    avg = str(round(total, 2))
>+  return avg

Nits:
1. exluding->excluding
2. val_list->values
3. there is no need to cast the members of val_list to float, we already do that inside the process_tpformat method
4. we should return a number, not a string (do the cast where the string value is actually used)

+# This is hacky and specific to the machines that we have for talos (which I tested on)
+# Please find a better library to get this information.  The key is we need specific strings
+# returned to match values in the AMO database
+def getOSDetails():
+  import subprocess
+  import platform
+  import re
+

The mozinfo library has functions to do this, ideally we would use that here.

https://github.com/mozilla/mozbase/tree/master/mozinfo

>+# sample data structure:
>+# data = {'addon': 2292, 'average': 0.1, 'appversion': 1, 'osversion': 1, 'test': 'ts'}
>+def uploadAMOResults(addonid, appversion, testname, vals):

'upload_amo_results' would be more PEP8 compliant (and consistent with
the other new code we're adding to talos).

>+  #TODO: define osvesion to match https://bugzilla.mozilla.org/show_bug.cgi?id=693209 and/or https://bugzilla.mozilla.org/show_bug.cgi?id=675117#c4
>+  osversion = "3.14"
>+
>+  data = {'addon': addonid,
>+          'average': avg_excluding_max(vals),
>+          'appversion': appversion,
>+          'osversion': osversion,
>+          'test': testname}
>+  print "DEBUG: in uploadAMOResults: %s" % data
>+
>+  #TODO: determine consumer_key/secret and how this can be checked into an open source repository.
>+  amo = AMOOAuth(domain="addons-dev.allizom.org", port=443, protocol='https',
>+                 prefix='/z')
>+  return amo.create_perf(data)

Should we make the domain (and/or prefix) configurable? Also, don't we need to set some sort of credentials via the set_consumer to be authorized to upload the information?

>diff --git a/talos/amo/amo_utils.py b/talos/amo/amo_utils.py
>new file mode 100644
>--- /dev/null
>+++ b/talos/amo/amo_utils.py
>@@ -0,0 +1,51 @@
>+import mimetypes
>+import os
>+
>+
>+def to_str(s):
>+    if isinstance(s, unicode):
>+        return s.encode('utf-8', 'strict')
>+    else:
>+        return str(s)

This seems to be an internally used method, could we call it _to_str ?

+def encode_multipart(boundary, data):
+    """Ripped from django."""
+    lines = []
+

Could we not use the docstring from the Django version of
encode_multipart
(http://code.djangoproject.com/svn/django/trunk/django/test/client.py). Here
it is:

    """
    Encodes multipart POST data from a dictionary of form values.

    The key will be used as the form data name; the value will be transmitted
    as content. If the value is a file, the contents of the file will be sent
    as an application/octet-stream; otherwise, str(value) will be sent.
    """

Strictly speaking, to comply with the Django license, we should include a copy of http://code.djangoproject.com/svn/django/trunk/LICENSE
in the file as well. That along with a quick link/note elsewhere in the file, should be sufficient documentation as to origin.

>diff --git a/talos/run_tests.py b/talos/run_tests.py
>--- a/talos/run_tests.py
>+++ b/talos/run_tests.py
>@@ -250,16 +250,24 @@ def send_to_graph(results_server, result
>         for line in page_results:
>           val, page = process_tpformat(line)
>           if val > -1 :
>             vals.append([val, page])
>     else:
>       raise talosError("Unknown print format in send_to_graph")
>     result_strings.append(construct_results(machine, fullname, browser_config, date, vals, amo))
>     result_testnames.append(fullname)
>+
>+    #TODO: do we only test ts, if not, can we ensure that we are not trying to uplaod ts_rss, etc...
>+    if amo and testname == 'ts':
>+      sys.path.insert(0, "amo")
>+      from amo_api import uploadAMOResults
>+      val_list = [val for val,page in vals]
>+      uploadAMOResults(browser_config['addon_id'], browser_config['browser_version'], testname, val_list)

A few things:

1. Why do we do the sys.path.insert?
2. from amo_api import uploadAMOResults should be at the top of the file (according to PEP8).
3. It's your call, but I'd just put [val for val,page in vals] in the arguments to the function.
Attachment #577217 - Flags: review?(wlachance) → review-
Depends on: 699129
(In reply to William Lachance (:wlach) from comment #16)
> Comment on attachment 577217 [details] [diff] [review] [diff] [details] [review]
> patch to upload amo results using the amo api (1.0)
> 
> It might be good to get some more feedback on the addon API parts from
> people more familiar with that code than me.
> 

> +# This is hacky and specific to the machines that we have for talos (which
> I tested on)
> +# Please find a better library to get this information.  The key is we need
> specific strings
> +# returned to match values in the AMO database
> +def getOSDetails():
> +  import subprocess
> +  import platform
> +  import re
> +
> 
> The mozinfo library has functions to do this, ideally we would use that here.
> 
I don't want to gate this on moving talos to use mozinfo.  I'd prefer we do something suboptimal here for now and get this finished and then land mozinfo and change it later.  
> 
> 2. from amo_api import uploadAMOResults should be at the top of the file
> (according to PEP8).
Why when this is the only place we use it?  I think it makes sense that only in the cases where we run the talos addons system we import this and use this API.
(In reply to Clint Talbert ( :ctalbert ) from comment #17)
> (In reply to William Lachance (:wlach) from comment #16)

> > 2. from amo_api import uploadAMOResults should be at the top of the file
> > (according to PEP8).
> Why when this is the only place we use it?  I think it makes sense that only
> in the cases where we run the talos addons system we import this and use
> this API.

As we discussed the nice thing about putting imports at the top is that we'll notice immediately if the environment we're running in isn't set up correctly, instead of just (as in this example) when we're doing a test that involves using the amo_api.
Bear in mind the amo-oauth module https://github.com/andymckay/amo-oauth is a sample module, used for quickly prototyping and proving that the AMO OAuth API is functioning without that all that mucking about setting up OAuth tokens. Please feel free to rip the bits you need out of it, but I wouldn't recommend relying on it completely as at this stage its rather raw and is used to test a number of different OAuth clients.
I have updated the bits and seem to be unable to test.  I am having trouble with the server connection piece:
Traceback (most recent call last):
  File "run_tests.py", line 603, in <module>
    main()
  File "run_tests.py", line 600, in main
    test_file(arg, options.screen, options.amo)
  File "run_tests.py", line 560, in test_file
    links = send_to_graph(results_server, results_link, title, date, browser_config, results, amo)
  File "run_tests.py", line 267, in send_to_graph
    [val for val,page in vals])
  File "amo/amo_api.py", line 65, in upload_amo_results
    retVal = amo.create_perf(data)
  File "amo/amo.py", line 228, in create_perf
    return self._send(self.url('perf'), 'POST', data)
  File "amo/amo.py", line 203, in _send
    data=data)
  File "amo/amo.py", line 120, in _request
    headers=headers, body=data)
  File "/usr/lib/python2.7/dist-packages/httplib2/__init__.py", line 1436, in request
    (response, content) = self._request(conn, authority, uri, request_uri, method, body, headers, redirections, cachekey)
  File "/usr/lib/python2.7/dist-packages/httplib2/__init__.py", line 1188, in _request
    (response, content) = self._conn_request(conn, request_uri, method, body, headers)
  File "/usr/lib/python2.7/dist-packages/httplib2/__init__.py", line 1123, in _conn_request
    conn.connect()
  File "/usr/lib/python2.7/dist-packages/httplib2/__init__.py", line 911, in connect
    raise SSLHandshakeError(e)
httplib2.SSLHandshakeError: [Errno 1] _ssl.c:503: error:14090086:SSL routines:SSL3_GET_SERVER_CERTIFICATE:certificate verify failed
jmaher@jmaher-ubuntu-mbp:~/mozilla/talos/talos/talos$
Is that a connection to addons-dev.allizom.org?
yeah:
  amo = AMOOAuth(domain="addons-dev.allizom.org", port=443, protocol='https',
                 prefix='/z')
Attached file cacerts.txt — 

    
    

    
  
Try adding cacerts.txt your /usr/lib/python2.7/dist-packages/httplib2/ directory.

~/sandboxes/amo-oauth/src(master) $ ls -al /usr/local/Cellar/python/2.7.2/lib/python2.7/site-packages/httplib2/ | grep cacerts.txt
-rw-r--r--   1 andy  admin  33640 16 Jun 07:29 cacerts.txt

This file is available in other places, I've attached my copy to the bug. A similar file from a more trustworthy source should work :)

    
    

    
  
See also: http://viraj-workstuff.blogspot.com/2011/07/python-httplib2-certificate-verify.html

You can (untested) also do httplib2.Http(disable_ssl_certificate_validation=True)

If this is going to be userland software, adding cacerts.txt to the global python install is fairly invasive.  If this is going to be deployed software, then this step will need to be done and maintained on all applicable machines (which are probably many).  If this is going to be both, then problem x 2. I would rather work around ssl validation in another way, if applicable.

(If comment 24 is only wrt testing, then that is another matter too. But it would be nice to have a better longer-term solution.)

    
    

    
  
addons-dev is using a legitimate purchased certificate.  There shouldn't be any verification problems.  I'm CCing oremj to see if there is anything obvious from his POV.

    
    

    
  
hmm, I already had a cacerts.txt file and the only difference was the one I had included some godaddy certs.  I tried the disable_ssl-certificate_validation (including the hack in the original amo-oauth github project), and now I end up with:

!!!!!!!!!!!!!!!!!
{'platform': 'x86', 'addon_id': '92383', 'version': '3.0.0', 'test': 'ts', 'average': 725.0, 'product_version': '11.0a1', 'os': 'Linux', 'product': 'firefox'}
!!!!!!!!!!!!!!!!!
Traceback (most recent call last):
  File "run_tests.py", line 603, in <module>
    main()
  File "run_tests.py", line 600, in main
    test_file(arg, options.screen, options.amo)
  File "run_tests.py", line 560, in test_file
    links = send_to_graph(results_server, results_link, title, date, browser_config, results, amo)
  File "run_tests.py", line 267, in send_to_graph
    [val for val,page in vals])
  File "amo/amo_api.py", line 68, in upload_amo_results
    retVal = amo.create_perf(data)
  File "amo/amo.py", line 229, in create_perf
    return self._send(self.url('perf'), 'POST', data)
  File "amo/amo.py", line 206, in _send
    raise ValueError('%s: %s' % (resp.status, content))
ValueError: 401: Forbidden

Do I need to be [not] on a specific network?  
Could my consumer_key or consumer_secret be incorrect?

    
    

    
  


  
updated patch to address the majority of the review comments.

I am still importing amo_api and doing the sys.path.insert().  Using this requires oauth and that is not guaranteed to be installed on the talos slaves.  We can ensure it is installed on the addon slaves though.

I still need to use the disable_ssl stuff for testing, but we shouldn't be doing that for production.

        Attachment #577217 -
        Attachment is obsolete: true

        Attachment #581698 -
        Flags: review?(wlachance)

    
    

    
  
Comment on attachment 581698 [details] [diff] [review]
patch to upload amo results using the amo api (1.1)

As discussed on irc, r+ if we insert this text:

http://code.djangoproject.com/svn/django/trunk/LICENSE

into amo_utils.py as a comment (for encode_multipart.py)

    
    

    
  
landed:
http://hg.mozilla.org/build/talos/rev/1ca80b0dbdb9
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED

    
    

    
  
Comment on attachment 581698 [details] [diff] [review]
patch to upload amo results using the amo api (1.1)

cancelling r? since it was mentioned r=me in a comment.

        Attachment #581698 -
        Flags: review?(wlachance)

    
    

    
  
(In reply to Joel Maher (:jmaher) from comment #30)
> landed:
> http://hg.mozilla.org/build/talos/rev/1ca80

We'll have to remove mozinfo.py when Bug 707218 - write a script to create a talos.zip file with the dependencies from mozbase lands

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: