644654 - Pre-calculate min/max/average for endurance tests metrics

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Automation, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Currently these are calculated in the dashboard code. As these will not change it makes more sense to do the work only once, so we will look to bring it into the automation script.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Pre-calculate min/max/average for endurance tests. v1

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 526403 [details] [diff] [review]
Pre-calculate min/max/average for endurance tests. v1

Dave, I would like to see it more generalized. Whenever we add a new field to the list of checkpoint items we would have to rewrite this code. That's really something we have to avoid.

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Pre-calculate min/max/average for endurance tests. v2

(In reply to comment #2)
> Comment on attachment 526403 [details] [diff] [review] [review]
> Pre-calculate min/max/average for endurance tests. v1
> 
> Dave, I would like to see it more generalized. Whenever we add a new field to
> the list of checkpoint items we would have to rewrite this code. That's really
> something we have to avoid.

Good point. Could you take a look at the new version of the patch and let me know if this is more what you had in mind?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 529887 [details] [diff] [review]
Pre-calculate min/max/average for endurance tests. v2

>+        metrics = ('allocated', 'mapped')
>+
>+        all_metrics = {}
>+
>+        for metric in metrics:
>+            all_metrics[metric] = []

In this case we also have a hard-coded list of properties to check. It looks better now, but can't you just retrieve the individual entries from the data array itself? That way we would never have to change the code.

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

So each checkpoint contains key/value for: timestamp, label, allocated, and mapped. Are you suggesting getting all of these keys and filtering out timestamp and label? This would mean that anything else we added here that wouldn't be relevant for stats generation would also need to be filtered out so there's still a maintenance issue.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ah, right. Nearly missed that. But IMO having a blacklist would be more appropriate as having to whitelist properties. Most of the data we want/can add, we should be able to calculate min/max/avg that way. What do you think?

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Pre-calculate min/max/average for endurance tests. v2.1

(In reply to comment #6)
> Ah, right. Nearly missed that. But IMO having a blacklist would be more
> appropriate as having to whitelist properties. Most of the data we want/can
> add, we should be able to calculate min/max/avg that way. What do you think?

I agree. Done.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 531494 [details] [diff] [review]
Pre-calculate min/max/average for endurance tests. v2.1

>+        checkpoint = report['endurance']['results'][0]['iterations'][0]['checkpoints'][0]

If we do not have checkpoints, i.e. if there was a test failure we will fail with this code. There is no try/catch.

>+            for iteration in test['iterations']:
>+                iteration_metrics = {}
>+
>+                iteration['stats'] = {}
>+                for metric in metrics:
>+                    iteration_metrics[metric] = []

The definition of iteration_metrics should be inside the metrics loop if needed by my next comment.

>+                    for checkpoint in iteration['checkpoints']:
>+                        iteration_metrics[metric].append(checkpoint[metric])
>+
>+                    iteration['stats'][metric] = {'average' : sum(iteration_metrics[metric]) / len(iteration_metrics[metric]),
>+                                                     'min' : min(iteration_metrics[metric]),
>+                                                     'max' : max(iteration_metrics[metric])}

Why do we have to fill the temporary array iteration_metrics first? Can't we directly run the calculation on iterations['checkpoints'][metric]? Also please correct the indentation of the last two lines so they align with average.

>+                test['stats'][metric] = {'average' : sum(test_metrics[metric]) / len(test_metrics[metric]),
>+                                            'min' : min(test_metrics[metric]),
>+                                            'max' : max(test_metrics[metric])}

We have that code multiple times. Please create a function which returns a dict and could internally self-iterate over all keys. It can replace all three instances.

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to comment #8)
> Comment on attachment 531494 [details] [diff] [review] [review]
> Pre-calculate min/max/average for endurance tests. v2.1
> 
> >+        checkpoint = report['endurance']['results'][0]['iterations'][0]['checkpoints'][0]
> 
> If we do not have checkpoints, i.e. if there was a test failure we will fail
> with this code. There is no try/catch.

Could you give an example of how you would do this? I was thinking of iterating over the report['endurance']['results'] until ['iterations'][0]['checkpoints'][0] doesn't throw an exception, but I'm a bit lost on how to code this up. It would need to silently catch the exceptions, and then ultimately throw an exception if no tests had iterations/checkpoints...

> >+            for iteration in test['iterations']:
> >+                iteration_metrics = {}
> >+
> >+                iteration['stats'] = {}
> >+                for metric in metrics:
> >+                    iteration_metrics[metric] = []
> 
> The definition of iteration_metrics should be inside the metrics loop if
> needed by my next comment.

I don't understand. As with the others, the iterations_metrics is defined inside the appropriate scope. Could you please explain your thoughts?

> >+                    for checkpoint in iteration['checkpoints']:
> >+                        iteration_metrics[metric].append(checkpoint[metric])
> >+
> >+                    iteration['stats'][metric] = {'average' : sum(iteration_metrics[metric]) / len(iteration_metrics[metric]),
> >+                                                     'min' : min(iteration_metrics[metric]),
> >+                                                     'max' : max(iteration_metrics[metric])}
> 
> Why do we have to fill the temporary array iteration_metrics first? Can't we
> directly run the calculation on iterations['checkpoints'][metric]? Also
> please correct the indentation of the last two lines so they align with
> average.

iterations['checkpoints'] is a list of checkpoints, each one is a dict. We need to add each of these to the iteration_metrics list so we can then calculate the iteration stats.

> >+                test['stats'][metric] = {'average' : sum(test_metrics[metric]) / len(test_metrics[metric]),
> >+                                            'min' : min(test_metrics[metric]),
> >+                                            'max' : max(test_metrics[metric])}
> 
> We have that code multiple times. Please create a function which returns a
> dict and could internally self-iterate over all keys. It can replace all
> three instances.

Good idea. I've implemented this, and will provide a patch once I have clarification/feedback on the other items in this comment.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #9)
> Could you give an example of how you would do this? I was thinking of
> iterating over the report['endurance']['results'] until
> ['iterations'][0]['checkpoints'][0] doesn't throw an exception, but I'm a
> bit lost on how to code this up. It would need to silently catch the
> exceptions, and then ultimately throw an exception if no tests had
> iterations/checkpoints...

We only need that code at this position to retrieve the list of available metrics. If we could move that inside the inner loop we could get rid of this code at all. Then we can check if we know which metrics are available and store the list in an array, so we do not have to do it for each checkpoint.

> > >+            for iteration in test['iterations']:
> > >+                iteration_metrics = {}
> > >+
> > >+                iteration['stats'] = {}
> > >+                for metric in metrics:
> > >+                    iteration_metrics[metric] = []
> > 
> > The definition of iteration_metrics should be inside the metrics loop if
> > needed by my next comment.
> 
> I don't understand. As with the others, the iterations_metrics is defined
> inside the appropriate scope. Could you please explain your thoughts?

iteration_metrics is not used outside of 'for metric in metrics:'.

> iterations['checkpoints'] is a list of checkpoints, each one is a dict. We
> need to add each of these to the iteration_metrics list so we can then
> calculate the iteration stats.

Hm, so as I read the numbers we need are the keys? If yes, can't we simply use dict.keys to retrieve all the data?

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Pre-calculate min/max/average for endurance tests. v3.0

(In reply to comment #10)
> (In reply to comment #9)
> > Could you give an example of how you would do this? I was thinking of
> > iterating over the report['endurance']['results'] until
> > ['iterations'][0]['checkpoints'][0] doesn't throw an exception, but I'm a
> > bit lost on how to code this up. It would need to silently catch the
> > exceptions, and then ultimately throw an exception if no tests had
> > iterations/checkpoints...
> 
> We only need that code at this position to retrieve the list of available
> metrics. If we could move that inside the inner loop we could get rid of
> this code at all. Then we can check if we know which metrics are available
> and store the list in an array, so we do not have to do it for each
> checkpoint.

I think I understood this correctly, and have implemented a solution.

> > > >+            for iteration in test['iterations']:
> > > >+                iteration_metrics = {}
> > > >+
> > > >+                iteration['stats'] = {}
> > > >+                for metric in metrics:
> > > >+                    iteration_metrics[metric] = []
> > > 
> > > The definition of iteration_metrics should be inside the metrics loop if
> > > needed by my next comment.
> > 
> > I don't understand. As with the others, the iterations_metrics is defined
> > inside the appropriate scope. Could you please explain your thoughts?
> 
> iteration_metrics is not used outside of 'for metric in metrics:'.

That's true, but it needs to be cleared after each iteration.

> > iterations['checkpoints'] is a list of checkpoints, each one is a dict. We
> > need to add each of these to the iteration_metrics list so we can then
> > calculate the iteration stats.
> 
> Hm, so as I read the numbers we need are the keys? If yes, can't we simply
> use dict.keys to retrieve all the data?

That's what we're doing, but we're filtering out the timestamp and label.

Comment 12

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Pre-calculate min/max/average for endurance tests. v3.1

Removed debugging.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 532936 [details] [diff] [review]
Pre-calculate min/max/average for endurance tests. v3.1

>+            for iteration in test['iterations']:
>+                iteration_metrics = {}
>+
>+                for checkpoint in iteration['checkpoints']:

Ok, it makes sense to define and reset iteration_metrics outside of the inner for loop. We only want to reset it per iteration.

>+                    if not metrics:
>+                        metrics = [key for key in checkpoint.keys() if not key in blacklist]
>+                    for metric in metrics:

nit: Please add an empty line before the next loop.

>+                        if metric in iteration_metrics:
>+                            iteration_metrics[metric].append(checkpoint[metric])
>+                        else:
>+                            iteration_metrics[metric] = [checkpoint[metric]]
>+                        if metric in test_metrics:
>+                            test_metrics[metric].extend(iteration_metrics[metric])
>+                        else:
>+                            test_metrics[metric] = iteration_metrics[metric]
>+                        if metric in all_metrics:
>+                            all_metrics[metric].extend(test_metrics[metric])
>+                        else:
>+                            all_metrics[metric] = test_metrics[metric]

Please document those lines. It's really hard to understand this logic. Also wouldn't it make sense to move the last two if/else cases out of the inner loop and put them right in-front of the appropriate calculate_stats calls? It looks kinda heavy to do for each checkpoint iteration.

>+                iteration['stats'] = self.calculate_stats(iteration_metrics, metrics)
>+
>+            test['stats'] = self.calculate_stats(test_metrics, metrics)
>+
>+        report['endurance']['stats'] = self.calculate_stats(all_metrics, metrics)

That's sooo much cleaner now. Thanks.

r- for now because I think we have to improve the performance of the inner loop. If I'm wrong please re-ask for review on the same patch.

Comment 14

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Pre-calculate min/max/average for endurance tests. v3.2

(In reply to comment #13)
> Comment on attachment 532936 [details] [diff] [review] [review]
> Pre-calculate min/max/average for endurance tests. v3.1
> 
> >+            for iteration in test['iterations']:
> >+                iteration_metrics = {}
> >+
> >+                for checkpoint in iteration['checkpoints']:
> 
> Ok, it makes sense to define and reset iteration_metrics outside of the
> inner for loop. We only want to reset it per iteration.

Just to check, there was no action here, right?

> >+                    if not metrics:
> >+                        metrics = [key for key in checkpoint.keys() if not key in blacklist]
> >+                    for metric in metrics:
> 
> nit: Please add an empty line before the next loop.

Done.

> >+                        if metric in iteration_metrics:
> >+                            iteration_metrics[metric].append(checkpoint[metric])
> >+                        else:
> >+                            iteration_metrics[metric] = [checkpoint[metric]]
> >+                        if metric in test_metrics:
> >+                            test_metrics[metric].extend(iteration_metrics[metric])
> >+                        else:
> >+                            test_metrics[metric] = iteration_metrics[metric]
> >+                        if metric in all_metrics:
> >+                            all_metrics[metric].extend(test_metrics[metric])
> >+                        else:
> >+                            all_metrics[metric] = test_metrics[metric]
> 
> Please document those lines. It's really hard to understand this logic. Also
> wouldn't it make sense to move the last two if/else cases out of the inner
> loop and put them right in-front of the appropriate calculate_stats calls?
> It looks kinda heavy to do for each checkpoint iteration.

Let me know if this looks better now.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 533624 [details] [diff] [review]
Pre-calculate min/max/average for endurance tests. v3.2

>+                    for metric in metrics:
>+                        if metric in iteration_metrics:
>+                            # contains the metric as a key so we can append
>+                            iteration_metrics[metric].append(checkpoint[metric])
>+                        else:
>+                            # does not contain the metric as a key so we create a new list
>+                            iteration_metrics[metric] = [checkpoint[metric]]
>+
>+                for metric in metrics:
>+                    if metric in test_metrics:
>+                        # contains the metric as a key so we can extend
>+                        test_metrics[metric].extend(iteration_metrics[metric])
>+                    else:
>+                        # does not contain the metric as a key so we create a new list
>+                         test_metrics[metric] = iteration_metrics[metric]
>+
>+            for metric in metrics:
>+                if metric in all_metrics:
>+                    # contains the metric as a key so we can extend
>+                    all_metrics[metric].extend(test_metrics[metric])
>+                else:
>+                    # does not contain the metric as a key so we create a new list
>+                    all_metrics[metric] = test_metrics[metric]

Looks way better, but we still have a lot of duplicated code here. We should at least move out the last two loops into a function. If you can even include the first would be great. The only difference is data type of the source element.

We are kinda close to final now.

Comment 16

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Pre-calculate min/max/average for endurance tests. v3.3

(In reply to comment #15)
> Comment on attachment 533624 [details] [diff] [review] [review]
> Pre-calculate min/max/average for endurance tests. v3.2
> 
> >+                    for metric in metrics:
> >+                        if metric in iteration_metrics:
> >+                            # contains the metric as a key so we can append
> >+                            iteration_metrics[metric].append(checkpoint[metric])
> >+                        else:
> >+                            # does not contain the metric as a key so we create a new list
> >+                            iteration_metrics[metric] = [checkpoint[metric]]
> >+
> >+                for metric in metrics:
> >+                    if metric in test_metrics:
> >+                        # contains the metric as a key so we can extend
> >+                        test_metrics[metric].extend(iteration_metrics[metric])
> >+                    else:
> >+                        # does not contain the metric as a key so we create a new list
> >+                         test_metrics[metric] = iteration_metrics[metric]
> >+
> >+            for metric in metrics:
> >+                if metric in all_metrics:
> >+                    # contains the metric as a key so we can extend
> >+                    all_metrics[metric].extend(test_metrics[metric])
> >+                else:
> >+                    # does not contain the metric as a key so we create a new list
> >+                    all_metrics[metric] = test_metrics[metric]
> 
> Looks way better, but we still have a lot of duplicated code here. We should
> at least move out the last two loops into a function. If you can even
> include the first would be great. The only difference is data type of the
> source element.

Good idea - done!

Comment 17

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Pre-calculate min/max/average for endurance tests. v3.4

Forgot to qref :/

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 533967 [details] [diff] [review]
Pre-calculate min/max/average for endurance tests. v3.4

That looks great now! Only some minor nits before we can get it landed:

>     def update_report(self, report):
>+    def populate_metrics(self, dict, keys, data):
>+    def calculate_stats(self, data, keys):

Please put those functions in an alphabetical order inside the Endurance class.

>+        for key in keys:
>+            if key in dict:
>+                # contains the key so we can add to it

This 'if' is simple enough so we don't really need this comment.

Comment 19

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Landed as:
http://hg.mozilla.org/qa/mozmill-automation/rev/b92a4822702d