Skip to content

Fix Bug 915317, display total volume of gccrashes #1900

Merged
1 commit merged into from Feb 25, 2014

3 participants

@ghost
ghost commented Feb 19, 2014
@rhelmer rhelmer commented on the diff Feb 19, 2014
...hstats/crashstats/templates/crashstats/gccrashes.html
@@ -61,7 +73,9 @@ <h3 class="title">Select Report Criteria</h3>
<figcaption class="title">Total Volume of GC Crashes</figcaption>
<section class="body">
<div id="graph_legend"></div>
- <div id="gccrashes_graph"></div>
+ <div id="gccrashes_graph">
+ <svg class="graph"></svg>
+ </div>
</section>
</figure>
</section>
@rhelmer
Mozilla member
rhelmer added a note Feb 19, 2014

You should mention somewhere that the graph is crashes-per-million-ADI

@rhelmer
Mozilla member
rhelmer added a note Feb 19, 2014

Also mention that these are "by buildID" and not dates (from lonnen)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@peterbe peterbe and 1 other commented on an outdated diff Feb 19, 2014
...stats/crashstats/static/crashstats/css/gccrashes.less
@@ -63,3 +62,16 @@ header {
width: 63%;
vertical-align: top;
}
+.gccrashes_graph {
+ width: 100%;
+ height: 550px;
+}
+.graph-tooltip-header {
+ background-color: #f0f0f0;
@peterbe
peterbe added a note Feb 19, 2014

Isn't the "standard" 4 spaces?

@ghost
ghost added a note Feb 19, 2014

ugh, yes it is. My bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@peterbe peterbe and 1 other commented on an outdated diff Feb 19, 2014
.../crashstats/static/crashstats/js/socorro/gccrashes.js
- selectedProduct.on('change', function() {
+ /**
+ * Takes a specified array of form fields and returns an
+ * encoded array ready for use in Ajax calls.
+ * @param {array} fields - Array of field objects
+ */
+ function getParams(fields) {
+ var fieldsLength = fields.length;
+ var params = [];
+
+ for (var i = 0; i < fieldsLength; i++) {
@peterbe
peterbe added a note Feb 19, 2014

Considering that you rely on jQuery (line 32) this can be replaced with:
$.each(fields, function(value, name) {

@peterbe
peterbe added a note Feb 19, 2014

Oops. I think it's $.each(fields, function(index, field) {

@ghost
ghost added a note Feb 19, 2014

Why not ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@peterbe peterbe commented on an outdated diff Feb 19, 2014
.../crashstats/static/crashstats/js/socorro/gccrashes.js
+ // Dates displayed in the UI is not in a format accepted by the middleware so,
+ // if a field is of type=date, convert it to ISO_STANDARD before adding to
+ // parameter object.
+ if (fields[i].attr('type') === 'date') {
+ value = socorro.date.formatDate(new Date(fields[i].val()), "ISO_STANDARD");
+ }
+
+ params.push({
+ name: name,
+ value: value
+ });
+ }
+ return $.param(params);
+ }
+
+ var baseUrl = '/gccrashes/json_data/';
@peterbe
peterbe added a note Feb 19, 2014

Can this please come from the template instead? Hardcoding URLs should be avoided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@peterbe peterbe and 1 other commented on an outdated diff Feb 19, 2014
...hstats/crashstats/templates/crashstats/gccrashes.html
<link rel="stylesheet" type="text/less" href="{{ static('crashstats/css/gccrashes.less') }}" media="screen" />
+ <link rel="stylesheet" type="text/css" href="{{ static('crashstats/css/lib/nv.d3.css') }}" media="screen" />
@peterbe
peterbe added a note Feb 19, 2014

Can you break this one out on its own.
If you use nv.d3.css in multiple places (other than just gccrashes) then we run the risk of downloading the same content again but under a different name.

@ghost
ghost added a note Feb 21, 2014

As in, into it's own compress block?

@peterbe
peterbe added a note Feb 21, 2014

Use your judgement. My point is that if you have...
...on one page:

{% compress css %}
<link href="nv.d3.css"><link href="foo.css">
{% endcompress %}

...and on some other different page...:

{% compress css %}
<link href="nv.d3.css"><link href="Different.css">
{% endcompress %}

Then, their combined names will be different and thus the user will download the content of nv.d3.css twice and no browser cache in the world will help him.
I think this is more important than to reduce the number different files. The optimization trick of clumping lots of files into one is more necessary when your visitors just visit one page and not likely come back to click multiple pages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@peterbe peterbe commented on an outdated diff Feb 19, 2014
...hstats/crashstats/templates/crashstats/gccrashes.html
@@ -21,7 +23,17 @@ <h3 class="title">Select Report Criteria</h3>
<form id="gccrashes" name="gccrashes" method="post" class="body">
<input type="hidden" name="csrfmiddlewaretoken" value="{{ csrf_token }}">
<fieldset>
- <legend class="visually-hidden">Select Product and Version for Graph</legend>
+ <legend class="visually-hidden">Select Report Criteria</legend>
+
+ <div class="field_elem_container">
+ <label for="start_date">From:</label>
+ <input type="date" name="start_date" id="start_date" value="{{ start_date }}" required />
@peterbe
peterbe added a note Feb 19, 2014

Is the id really needed? I think $('#gccrashes input[name="start_date"]') will be enough.

@peterbe
peterbe added a note Feb 19, 2014

Ah! I see> It's for the label. Ignore me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@peterbe peterbe and 1 other commented on an outdated diff Feb 19, 2014
...hstats/crashstats/templates/crashstats/gccrashes.html
{% compress js %}
- <script src="{{ static('crashstats/js/flot-0.7/jquery.flot.pack.js') }}"></script>
+ <script src="{{ static('crashstats/js/lib/d3.v3.js') }}"></script>
@peterbe
peterbe added a note Feb 19, 2014

This is used in the crontabber-state page too. I think we should correct this template and that template so that we don't accidentally have to download it twice.

@ghost
ghost added a note Feb 19, 2014

I saw there was d3 related file(s) in the root of JS but was not sure whether these are used so, I added my dependencies into a lib folder where I believe they should live.

@ghost
ghost added a note Feb 21, 2014

Should I change the crontabber state files so they use the d3 files from js/lib? I suppose I can then remove the remaining one's in the root of the js folder.

@peterbe
peterbe added a note Feb 21, 2014

Yes, feel free to change the crontabber-state template too. I quite like the use of the sub-directory "lib" too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@peterbe peterbe commented on the diff Feb 19, 2014
.../crashstats/static/crashstats/js/socorro/gccrashes.js
+ // if a field is of type=date, convert it to ISO_STANDARD before adding to
+ // parameter object.
+ if (field.attr('type') === 'date') {
+ value = socorro.date.formatDate(new Date(field.val()), "ISO_STANDARD");
+ }
+
+ params.push({
+ name: name,
+ value: value
+ });
+ });
+
+ return $.param(params);
+ }
+
+ var baseUrl = '/gccrashes/json_data/';
@peterbe
peterbe added a note Feb 19, 2014

I'd much rather see this being something like var baseUrl = $('#gcccrashes').data('base-url') and in the template do <div id="gccrashes" data-base-url="{{ static('crashstats.gccrashes_json_data')}} ">

@ghost
ghost added a note Feb 21, 2014

@peterbe So, the problem is this. Using static does not work, as it is expecting to load a resource from STATIC_ROOT so, all it outputs is /static/gccrashes.json

Now, if I try to use url() I run into the problem that the pattern in urls.py expects a product and version passed to the view so, I need to do something like below or I get a NoReverseMatch error:

url('crashstats.gccrashes_json', product, selected_version)

The problem with that though is it returns gccrashes/json_data but with products/Firefox/versions/30.0a1 appended. Because the product/version can change before the next Ajax request, having this part of the baseURL will not work.

I suppose I can have another pattern in urls.py for gccrashes_json that does not expect anything passed to it, but I am unsure if that is a good idea. Should I just go ahead and do that? With all that said though, cool URLs don't change right ;)

@peterbe
peterbe added a note Feb 26, 2014

Yeah, I meant url not static. Anyway...

I see your dilemma. But let's get back to it momentarily.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@peterbe peterbe commented on the diff Feb 19, 2014
webapp-django/crashstats/crashstats/views.py
- if not versions:
- versions = []
- for release in context['currentversions']:
- if release['product'] == product:
- if release['release'].lower() == 'nightly':
- versions.append(release['version'])
+ versions = []
+ for release in default_context['currentversions']:
+ rel_product = release['product']
+ rel_release = release['release'].lower()
+ if rel_product == product:
+ if rel_release in [x.lower() for x in nightlies_only]:
+ versions.append(release['version'])
@peterbe
peterbe added a note Feb 19, 2014

These two lines can be re-written as:

versions.extend([x.lower() for x in nighlies_only if rel_release == x])
@peterbe
peterbe added a note Feb 19, 2014

...but perhaps not necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@peterbe peterbe commented on an outdated diff Feb 19, 2014
webapp-django/crashstats/crashstats/views.py
context['products'] = current_products
+ context['selected_version'] = version
+ context['selected_product'] = product
+
+ date_format = '%Y/%m/%d'
+ date_today = datetime.datetime.utcnow()
+ date_week_ago = date_today - datetime.timedelta(days=7)
+
+ context['start_date'] = date_week_ago.strftime(date_format)
@peterbe
peterbe added a note Feb 19, 2014

It appears my comment disappeared from before, but I strongly encourage you to not do this level of presentation details in the view. You should probably just put the datetime.datetime object in the context and then something like <input value="{{ start_date.strftime('%Y/%m/%d') }}" in the template.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@peterbe
peterbe commented Feb 19, 2014

I have only poked around with the code which now looks good. Some nits needs addressing.
I'll leave it to @rhelmer to actually test it out and make sure it produces a pretty graph :)

@rhelmer rhelmer commented on an outdated diff Feb 24, 2014
...hstats/crashstats/templates/crashstats/gccrashes.html
@@ -58,10 +73,11 @@ <h3 class="title">Select Report Criteria</h3>
<section class="panel report-graph">
<figure id="graph-figure">
- <figcaption class="title">Total Volume of GC Crashes</figcaption>
+ <figcaption class="title">Total Volume of GC Crashes by 1M ADI by Build Day</figcaption>
@rhelmer
Mozilla member
rhelmer added a note Feb 24, 2014

I'd just say Build ID here, since there can be more than one per day. It's really a build ID, just happens to be a timestamp also right now (of when the build started) :)

r+ from me, with that change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ghost ghost merged commit 164049c into mozilla:master Feb 25, 2014

1 check passed

Details default Jenkins build 'socorro-github' #2849 has succeeded
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.