bug 1344864: refactored topcrashers

why:
- needed to extract some of the functionality used to generate topcrashers data because part of it will be used to generate signature summary data

what:
- refacted topcrashers
  - started generating data through classes instead of a dictionary
  - extracted computations into methods

webapp-django/crashstats/base/utils.py

@@ -77,3 +77,121 @@ def requests_retry_session(
     session.mount('http://', adapter)
     session.mount('https://', adapter)
     return session
+
+
+def get_signatures_stats(results, previous_range_results, platforms):
+    signatures = results['facets']['signature']
+    num_signatures = results['total']
+    platform_codes = [x['code'] for x in platforms if x['code'] != 'unknown']
+
+    signatures_stats = []
+    for i, signature in enumerate(signatures):
+        signatures_stats.append(SignatureStats(signature, i, num_signatures, platform_codes))
+
+    if num_signatures > 0 and 'signature' in previous_range_results['facets']:
+        previous_signatures = get_comparison_signatures(previous_range_results)
+        for signature_stats in signatures_stats:
+            previous_signature = previous_signatures.get(signature_stats.signature)
+            if previous_signature:
+                signature_stats.diff = previous_signature['percent'] - signature_stats.percent
+                signature_stats.rank_diff = previous_signature['rank'] - signature_stats.rank
+                signature_stats.previous_percent = previous_signature['percent']
+            else:
+                signature_stats.diff = 'new'
+                signature_stats.rank_diff = 0
+                signature_stats.previous_percent = 0
+
+    return signatures_stats
+
+
+class SignatureStats:
+    def __init__(self, signature, rank, num_signatures, platform_codes):
+        self.signature = signature['term']
+        self.rank = rank
+        self.percent = 100.0 * signature['count'] / num_signatures
+        self.count = signature['count']
+        self.platforms = get_num_crashes_per_platform(signature['facets']['platform'],
+                                                      platform_codes)
+        self.is_gc_count = get_num_crashes_in_garbage_collection(
+            signature['facets']['is_garbage_collecting'])
+        self.installs_count = (signature['facets']['cardinality_install_time']['value'])
+        self.startup_stats = SignatureStartupStats(signature)
+
+
+class SignatureStartupStats:
+    def __init__(self, signature):
+        self.count = signature['count']
+        self.plugin_count = get_num_plugin_crashes(signature['facets']['process_type'])
+        self.hang_count = get_num_hang_crashes(signature['facets']['hang_type'])
+        self.startup_count = get_num_startup_crashes(signature['facets']['startup_crash'])
+        self.startup_crash = get_is_startup_crash(signature['facets']['histogram_uptime'],
+                                                  signature['count'])
+
+
+def get_num_crashes_per_platform(platform_facet, platform_codes):
+    num_crashes_per_platform = {}
+    for platform in platform_codes:
+        num_crashes_per_platform[platform + '_count'] = 0
+    for platform in platform_facet:
+        code = platform['term'][:3].lower()
+        if code in platform_codes:
+            num_crashes_per_platform[code + '_count'] = platform['count']
+    return num_crashes_per_platform
+
+
+def get_num_crashes_in_garbage_collection(is_garbage_collecting_facet):
+    num_crashes_in_garbage_collection = 0
+    for row in is_garbage_collecting_facet:
+        if row['term'].lower() == 't':
+            num_crashes_in_garbage_collection = row['count']
+    return num_crashes_in_garbage_collection
+
+
+def get_num_plugin_crashes(process_type_facet):
+    num_plugin_crashes = 0
+    for row in process_type_facet:
+        if row['term'].lower() == 'plugin':
+            num_plugin_crashes = row['count']
+    return num_plugin_crashes
+
+
+def get_num_hang_crashes(hang_type_facet):
+    num_hang_crashes = 0
+    for row in hang_type_facet:
+        # Hangs have weird values in the database: a value of 1 or -1
+        # means it is a hang, a value of 0 or missing means it is not.
+        if row['term'] in (1, -1):
+            num_hang_crashes += row['count']
+    return num_hang_crashes
+
+
+def get_num_startup_crashes(startup_crash_facet):
+    return sum(
+        row['count'] for row in startup_crash_facet
+        if row['term'] in ('T', '1')
+    )
+
+
+def get_is_startup_crash(histogram_uptime_facet, crash_count):
+    is_startup_crash = False
+    for row in histogram_uptime_facet:
+        # Aggregation buckets use the lowest value of the bucket as
+        # term. So for everything between 0 and 60 excluded, the
+        # term will be `0`.
+        if row['term'] < 60:
+            ratio = 1.0 * row['count'] / crash_count
+            is_startup_crash = ratio > 0.5
+    return is_startup_crash
+
+
+def get_comparison_signatures(results):
+    signatures = results['facets']['signature']
+    num_signatures = results['total']
+    comparison_signatures = {}
+    for i, signature in enumerate(signatures):
+        comparison_signatures[signature['term']] = {
+            'count': signature['count'],
+            'rank': i + 1,
+            'percent': 100.0 * signature['count'] / num_signatures
+        }
+    return comparison_signatures

webapp-django/crashstats/topcrashers/jinja2/topcrashers/topcrashers.html

@@ -159,8 +159,8 @@ <h2>
                                 <input type="hidden" class='ajax-signature' name="ajax-signature-1" value="{{ crash.signature }}" />
                             </div>
                             <div class="signature-icons">
-                                {% if crash.startup_count > 0 %}
-                                {% if crash.startup_count == crash.count %}
+                                {% if crash.startup_stats.startup_count > 0 %}
+                                {% if crash.startup_stats.startup_count == crash.count %}
                                 <img src="{{ static('img/3rdparty/silk/flag_red.png') }}"
                                     alt="red flag"
                                     title="Startup Crash, all crashes happened during startup"
@@ -169,13 +169,13 @@ <h2>
                                 {% else %}
                                 <img src="{{ static('img/3rdparty/silk/flag_yellow.png') }}"
                                     alt="yellow flag"
-                                    title="Potential Startup Crash, {{ crash.startup_count }} out of {{ crash.count }} crashes happened during startup"
+                                    title="Potential Startup Crash, {{ crash.startup_stats.startup_count }} out of {{ crash.count }} crashes happened during startup"
                                     class="startup" height="16" width="16"
                                 />
                                 {% endif %}
                                 {% endif %}
 
-                                {% if crash.startup_crash %}
+                                {% if crash.startup_stats.startup_crash %}
                                 <img
                                     src="{{ static('img/icons/rocket_fly.png') }}"
                                     alt="rocket"
@@ -184,11 +184,11 @@ <h2>
                                 />
                                 {% endif %}
 
-                                {% if crash.hang_count > 0 %}
+                                {% if crash.startup_stats.hang_count > 0 %}
                                 <img src="{{ static('img/3rdparty/fatcow/stop16x16.png') }}" alt="stop sign" title="Hanged Crash" class="hang" height="16" width="16">
                                 {% endif %}
 
-                                {% if crash.plugin_count > 0 %}
+                                {% if crash.startup_stats.plugin_count > 0 %}
                                 <img src="{{ static('img/3rdparty/fatcow/brick16x16.png') }}" alt="brick" title="Plugin Crash" class="plugin" height="16" width="16">
                                 {% else %}
                                 <img src="{{ static('img/3rdparty/fatcow/application16x16.png') }}" width="16" height="16" alt="application" title="Browser Crash" class="browser" />
@@ -197,9 +197,9 @@ <h2>
                         </td>
                         {% if not os_name %}
                         <td>{{ crash.count }}</td>
-                        <td>{{ crash.win_count }}</td>
-                        <td>{{ crash.mac_count }}</td>
-                        <td>{{ crash.lin_count }}</td>
+                        <td>{{ crash.platforms.win_count }}</td>
+                        <td>{{ crash.platforms.mac_count }}</td>
+                        <td>{{ crash.platforms.lin_count }}</td>
                         {% elif os_name == 'Windows' %}
                         <td>{{ crash.win_count }}</td>
                         {% elif os_name == 'Linux' %}

webapp-django/crashstats/topcrashers/views.py

@@ -10,6 +10,7 @@
 
 from session_csrf import anonymous_csrf
 
+from crashstats.base.utils import get_signatures_stats
 from crashstats.crashstats import models
 from crashstats.crashstats.decorators import (
     check_days_parameter,
@@ -27,8 +28,6 @@ def datetime_to_build_id(date):
 
 def get_topcrashers_results(**kwargs):
     """Return the results of a search. """
-    results = []
-
     params = kwargs
     range_type = params.pop('_range_type')
     dates = get_date_boundaries(params)
@@ -59,77 +58,8 @@ def get_topcrashers_results(**kwargs):
     api = SuperSearchUnredacted()
     search_results = api.get(**params)
 
+    signatures_stats = []
     if search_results['total'] > 0:
-        results = search_results['facets']['signature']
-
-        platforms = models.Platforms().get_all()['hits']
-        platform_codes = [
-            x['code'] for x in platforms if x['code'] != 'unknown'
-        ]
-
-        for i, hit in enumerate(results):
-            hit['signature'] = hit['term']
-            hit['rank'] = i + 1
-            hit['percent'] = 100.0 * hit['count'] / search_results['total']
-
-            # Number of crash per platform.
-            for platform in platform_codes:
-                hit[platform + '_count'] = 0
-
-            sig_platforms = hit['facets']['platform']
-            for platform in sig_platforms:
-                code = platform['term'][:3].lower()
-                if code in platform_codes:
-                    hit[code + '_count'] = platform['count']
-
-            # Number of crashes happening during garbage collection.
-            hit['is_gc_count'] = 0
-            sig_gc = hit['facets']['is_garbage_collecting']
-            for row in sig_gc:
-                if row['term'].lower() == 't':
-                    hit['is_gc_count'] = row['count']
-
-            # Number of plugin crashes.
-            hit['plugin_count'] = 0
-            sig_process = hit['facets']['process_type']
-            for row in sig_process:
-                if row['term'].lower() == 'plugin':
-                    hit['plugin_count'] = row['count']
-
-            # Number of hang crashes.
-            hit['hang_count'] = 0
-            sig_hang = hit['facets']['hang_type']
-            for row in sig_hang:
-                # Hangs have weird values in the database: a value of 1 or -1
-                # means it is a hang, a value of 0 or missing means it is not.
-                if row['term'] in (1, -1):
-                    hit['hang_count'] += row['count']
-
-            # Number of crashes happening during startup. This is defined by
-            # the client, as opposed to the next method which relies on
-            # the uptime of the client.
-            hit['startup_count'] = sum(
-                row['count'] for row in hit['facets']['startup_crash']
-                if row['term'] in ('T', '1')
-            )
-
-            # Is a startup crash if more than half of the crashes are happening
-            # in the first minute after launch.
-            hit['startup_crash'] = False
-            sig_uptime = hit['facets']['histogram_uptime']
-            for row in sig_uptime:
-                # Aggregation buckets use the lowest value of the bucket as
-                # term. So for everything between 0 and 60 excluded, the
-                # term will be `0`.
-                if row['term'] < 60:
-                    ratio = 1.0 * row['count'] / hit['count']
-                    hit['startup_crash'] = ratio > 0.5
-
-            # Number of distinct installations.
-            hit['installs_count'] = (
-                hit['facets']['cardinality_install_time']['value']
-            )
-
         # Run the same query but for the previous date range, so we can
         # compare the rankings and show rank changes.
         delta = (dates[1] - dates[0]) * 2
@@ -150,30 +80,11 @@ def get_topcrashers_results(**kwargs):
             ]
 
         previous_range_results = api.get(**params)
-        total = previous_range_results['total']
-
-        compare_signatures = {}
-        if total > 0 and 'signature' in previous_range_results['facets']:
-            signatures = previous_range_results['facets']['signature']
-            for i, hit in enumerate(signatures):
-                compare_signatures[hit['term']] = {
-                    'count': hit['count'],
-                    'rank': i + 1,
-                    'percent': 100.0 * hit['count'] / total
-                }
-
-        for hit in results:
-            sig = compare_signatures.get(hit['term'])
-            if sig:
-                hit['diff'] = sig['percent'] - hit['percent']
-                hit['rank_diff'] = sig['rank'] - hit['rank']
-                hit['previous_percent'] = sig['percent']
-            else:
-                hit['diff'] = 'new'
-                hit['rank_diff'] = 0
-                hit['previous_percent'] = 0
+        platforms = models.Platforms().get_all()['hits']
 
-    return search_results
+        signatures_stats = get_signatures_stats(search_results, previous_range_results, platforms)
+
+    return signatures_stats
 
 
 @pass_default_context
@@ -298,20 +209,18 @@ def topcrashers(request, days=None, possible_days=None, default_context=None):
         _range_type=range_type,
     )
 
-    if api_results['total'] > 0:
-        tcbs = api_results['facets']['signature']
-    else:
-        tcbs = []
+    tcbs = api_results
 
     count_of_included_crashes = 0
     signatures = []
+
     for crash in tcbs[:int(result_count)]:
-        signatures.append(crash['signature'])
-        count_of_included_crashes += crash['count']
+        signatures.append(crash.signature)
+        count_of_included_crashes += crash.count
 
     context['number_of_crashes'] = count_of_included_crashes
-    context['total_percentage'] = api_results['total'] and (
-        100.0 * count_of_included_crashes / api_results['total']
+    context['total_percentage'] = len(api_results) and (
+        100.0 * count_of_included_crashes / len(api_results)
     )
 
     # Get augmented bugs data.
@@ -344,30 +253,30 @@ def topcrashers(request, days=None, possible_days=None, default_context=None):
                 continue
             os_code = operating_system['code'][0:3].lower()
             key = '%s_count' % os_code
-            crash_counts.append([crash[key], operating_system['name']])
+            crash_counts.append([crash.platforms[key], operating_system['name']])
 
-        sig = crash['signature']
+        sig = crash.signature
 
         # Augment with bugs.
         if sig in bugs:
-            if 'bugs' in crash:
-                crash['bugs'].extend(bugs[sig])
+            if hasattr(crash, 'bugs'):
+                crash.bugs.extend(bugs[sig])
             else:
-                crash['bugs'] = bugs[sig]
+                crash.bugs = bugs[sig]
 
         # Augment with first appearance dates.
         if sig in sig_date_data:
-            crash['first_report'] = sig_date_data[sig]
+            crash.first_report = sig_date_data[sig]
 
-        if 'bugs' in crash:
-            crash['bugs'].sort(reverse=True)
+        if hasattr(crash, 'bugs'):
+            crash.bugs.sort(reverse=True)
 
     context['tcbs'] = tcbs
     context['days'] = days
     context['report'] = 'topcrasher'
     context['possible_days'] = possible_days
     context['total_crashing_signatures'] = len(signatures)
-    context['total_number_of_crashes'] = api_results['total']
+    context['total_number_of_crashes'] = len(api_results)
     context['process_type_values'] = []
     for option in settings.PROCESS_TYPES:
         if option == 'all':