744403 - allow for configuration of server to send raw results to

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

currently in talos we specify the server to send data to.  This data is aggregated and summarized data.  We are working on transitioning to a server which accepts raw data and in doing this we will be double posting data.

I see this being an option in the config file: "raw_results_url: http://url".

Comment 1

[Fahim Zahur]

I am interested in working on this one. I had a couple of questions. Where is the server specified now? Will the server be specified in the command line? Thanks

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

We specify the server here:
http://hg.mozilla.org/build/talos/file/8ee9b373ea24/talos/run_tests.py#l244

It would be nice if we could do raw_results_url=http://1.2.3.4/path on the command line.  

Thanks for picking this up!

Comment 3

[Fahim Zahur]

Sorry for the delay in starting on this.

What I am thinking is - we want to remove the hard-coded server specified in the link you gave:

post_file.post_multipart("http://10.8.73.29", "/views/api/load_test",

When we do raw_results_url=http://1.2.3.4/path, "http:/1.2.3.4" will be the first argument of post_file.post_multipart and "/path" the second argument. 

Is that right? thanks!

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

yes, that is correct!

Comment 5

[Fahim Zahur]

Attachment: Allow raw server configuration

Comment 6

[Burak Yiğit Kaya [:BYK]]

Comment on attachment 629608 [details] [diff] [review]
Allow raw server configuration

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

Code looks good but may use some little refactoring. Also indentation inconsistencies are a no-no for Python code. So please make sure you fix them ;)

::: talos/PerfConfigurator.py
@@ +42,4 @@
>          ('resultsLink', {'help': 'Link to the results from this test run [DEPRECATED: use --resultsURL]',
>                           'flags': ['-l', '--resultsLink']}),
>          ('results_url', {'help': 'URL of results server or file:// url for local output'}),
> +		('raw_results_url', {'help': 'URL of raw results server'}),

I think we have an indentation problem here. Also the explanation should be "URL for the raw results server endpoint" since servers usually have a domain, instead of an URL. Subtle but important difference ;)

::: talos/run_tests.py
@@ +151,4 @@
>        options.append('"%s": "%s"' % (option, test[option]))
>    return "{%s}" % ', '.join(options)
>  
> +def send_to_graph(results_url, raw_results_url, machine, date, browser_config, results, amo, filters):

raw_results_url seems to be optional below. So instead of adding it here, why not add it to the end and assign a default(like None) to keep backwards compatibility? Also assigning a default is likely to shave some code off from the related parts in the function below.

@@ +254,5 @@
>  
>      raw_counters = '"results_aux": {%s}' % ', '.join(aux)
>      raw_results = "{%s, %s, %s, %s, %s}" % (raw_machine, raw_build, raw_testrun, raw_values, raw_counters)
> +	
> +	# ensure raw_results_url link exists

Indentation problem. Use 4 spaces, not tabs.

@@ +259,5 @@
> +    raw_results_scheme_server = "http://10.8.73.29"
> +    raw_results_path = "/views/api/load_test"
> +    if raw_results_url:
> +        raw_results_url_split = urlparse.urlsplit(raw_results_url)
> +        raw_results_scheme, raw_results_server, raw_results_split_path, _, _ = raw_results_url_split

Why using and intermediate variable? It is one time only.

@@ +261,5 @@
> +    if raw_results_url:
> +        raw_results_url_split = urlparse.urlsplit(raw_results_url)
> +        raw_results_scheme, raw_results_server, raw_results_split_path, _, _ = raw_results_url_split
> +        if raw_results_scheme in ('http', 'https') and post_file.link_exists(raw_results_server, raw_results_path):
> +            raw_results_scheme_server = raw_results_scheme +  raw_results_server	

raw_results_scheme_server is a very confusing variable name. It should be something like raw_results_schemed_server or raw_results_schemed_url or similar.

@@ +476,2 @@
>    amo = config.get('amo', False)
> +  

Please remove trailing spaces on empty lines if you are changing them ;)

Comment 7

[Fahim Zahur]

Attachment: Allow raw server configuration

My bad for the bad indentation on the last patch.

Comment 8

[Burak Yiğit Kaya [:BYK]]

Comment on attachment 629664 [details] [diff] [review]
Allow raw server configuration

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

Thanks for the fixes. I'm leaving it as a r? since I'm not sure about the part I commented on. I'll change my vote after getting some feedback from someone else.

::: talos/run_tests.py
@@ +259,5 @@
> +    raw_results_url_split = urlparse.urlsplit(raw_results_url)
> +    raw_results_scheme, raw_results_server, raw_results_path, _, _ = raw_results_url_split
> +    if raw_results_scheme in ('http', 'https') and not post_file.link_exists(raw_results_server, raw_results_path):
> +        raw_results_schemed_url = "http://10.8.73.29"	
> +        raw_results_path = "/views/api/load_test"

Is this part still necessary after using the default behavior? I mean it is still possible the user to provide some garbage as the URL but I'm not sure trying to fix that ambitiously is a good thing or not. Probably not.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 629664 [details] [diff] [review]
Allow raw server configuration

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

This is a simple patch and a simple problem to solve.  I think we just need to make sure we support the option to have None and not post as per below.

::: talos/PerfConfigurator.py
@@ +42,4 @@
>          ('resultsLink', {'help': 'Link to the results from this test run [DEPRECATED: use --resultsURL]',
>                           'flags': ['-l', '--resultsLink']}),
>          ('results_url', {'help': 'URL of results server or file:// url for local output'}),
> +        ('raw_results_url', {'help': 'URL for the raw results server endpoint'}),

it seems like we are missing something else in this file.  I would like to make this something that lives in a config file (now it is ttest.py), and eventually only used from commandline options.  If we don't specify a value, then it would not do anything and just ignore the entire results_raw posting.

::: talos/run_tests.py
@@ +259,5 @@
> +    raw_results_url_split = urlparse.urlsplit(raw_results_url)
> +    raw_results_scheme, raw_results_server, raw_results_path, _, _ = raw_results_url_split
> +    if raw_results_scheme in ('http', 'https') and not post_file.link_exists(raw_results_server, raw_results_path):
> +        raw_results_schemed_url = "http://10.8.73.29"	
> +        raw_results_path = "/views/api/load_test"

I agree as well, throw a message "WARNING: Talos is unable to recognize raw results url '%s'" % raw_results_url

Comment 10

[Jeff Hammel]

should be taken care of in the patch on https://bugzilla.mozilla.org/show_bug.cgi?id=755527

Comment 11

[Jeff Hammel]

fixed with bug 755527