Fix Bug 915317, display total volume of gccrashes #1900

Merged
1 commit merged into from Feb 25, 2014

Conversation

Projects
None yet
3 participants
@ghost

ghost commented Feb 19, 2014

- <div id="gccrashes_graph"></div>
+ <div id="gccrashes_graph">
+ <svg class="graph"></svg>
+ </div>
</section>
</figure>
</section>

This comment has been minimized.

@rhelmer

rhelmer Feb 19, 2014

Contributor

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

@rhelmer

rhelmer Feb 19, 2014

Contributor

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

This comment has been minimized.

@rhelmer

rhelmer Feb 19, 2014

Contributor

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

@rhelmer

rhelmer Feb 19, 2014

Contributor

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

@peterbe

View changes

webapp-django/crashstats/crashstats/static/crashstats/css/gccrashes.less
+ height: 550px;
+}
+.graph-tooltip-header {
+ background-color: #f0f0f0;

This comment has been minimized.

@peterbe

peterbe Feb 19, 2014

Contributor

Isn't the "standard" 4 spaces?

@peterbe

peterbe Feb 19, 2014

Contributor

Isn't the "standard" 4 spaces?

This comment has been minimized.

@ghost

ghost Feb 19, 2014

ugh, yes it is. My bad.

@ghost

ghost Feb 19, 2014

ugh, yes it is. My bad.

@peterbe

View changes

...p-django/crashstats/crashstats/static/crashstats/js/socorro/gccrashes.js
+ var fieldsLength = fields.length;
+ var params = [];
+
+ for (var i = 0; i < fieldsLength; i++) {

This comment has been minimized.

@peterbe

peterbe Feb 19, 2014

Contributor

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

@peterbe

peterbe Feb 19, 2014

Contributor

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

This comment has been minimized.

@peterbe

peterbe Feb 19, 2014

Contributor

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

@peterbe

peterbe Feb 19, 2014

Contributor

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

This comment has been minimized.

@ghost

ghost Feb 19, 2014

Why not ;)

@peterbe

View changes

...p-django/crashstats/crashstats/static/crashstats/js/socorro/gccrashes.js
+ return $.param(params);
+ }
+
+ var baseUrl = '/gccrashes/json_data/';

This comment has been minimized.

@peterbe

peterbe Feb 19, 2014

Contributor

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

@peterbe

peterbe Feb 19, 2014

Contributor

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

@peterbe

View changes

webapp-django/crashstats/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" />

This comment has been minimized.

@peterbe

peterbe Feb 19, 2014

Contributor

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.

@peterbe

peterbe Feb 19, 2014

Contributor

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.

This comment has been minimized.

@ghost

ghost Feb 21, 2014

As in, into it's own compress block?

@ghost

ghost Feb 21, 2014

As in, into it's own compress block?

This comment has been minimized.

@peterbe

peterbe Feb 21, 2014

Contributor

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.

@peterbe

peterbe Feb 21, 2014

Contributor

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.

@peterbe

View changes

webapp-django/crashstats/crashstats/templates/crashstats/gccrashes.html
+
+ <div class="field_elem_container">
+ <label for="start_date">From:</label>
+ <input type="date" name="start_date" id="start_date" value="{{ start_date }}" required />

This comment has been minimized.

@peterbe

peterbe Feb 19, 2014

Contributor

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

@peterbe

peterbe Feb 19, 2014

Contributor

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

This comment has been minimized.

@peterbe

peterbe Feb 19, 2014

Contributor

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

@peterbe

peterbe Feb 19, 2014

Contributor

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

@peterbe

View changes

webapp-django/crashstats/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>

This comment has been minimized.

@peterbe

peterbe Feb 19, 2014

Contributor

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.

@peterbe

peterbe Feb 19, 2014

Contributor

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.

This comment has been minimized.

@ghost

ghost 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 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.

This comment has been minimized.

@ghost

ghost 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.

@ghost

ghost 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.

This comment has been minimized.

@peterbe

peterbe Feb 21, 2014

Contributor

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

@peterbe

peterbe Feb 21, 2014

Contributor

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

+ return $.param(params);
+ }
+
+ var baseUrl = '/gccrashes/json_data/';

This comment has been minimized.

@peterbe

peterbe Feb 19, 2014

Contributor

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')}} ">

@peterbe

peterbe Feb 19, 2014

Contributor

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')}} ">

This comment has been minimized.

@ghost

ghost 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 ;)

@ghost

ghost 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 ;)

This comment has been minimized.

@peterbe

peterbe Feb 26, 2014

Contributor

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

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

@peterbe

peterbe Feb 26, 2014

Contributor

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

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

+ rel_release = release['release'].lower()
+ if rel_product == product:
+ if rel_release in [x.lower() for x in nightlies_only]:
+ versions.append(release['version'])

This comment has been minimized.

@peterbe

peterbe Feb 19, 2014

Contributor

These two lines can be re-written as:

versions.extend([x.lower() for x in nighlies_only if rel_release == x])
@peterbe

peterbe Feb 19, 2014

Contributor

These two lines can be re-written as:

versions.extend([x.lower() for x in nighlies_only if rel_release == x])

This comment has been minimized.

@peterbe

peterbe Feb 19, 2014

Contributor

...but perhaps not necessary.

@peterbe

peterbe Feb 19, 2014

Contributor

...but perhaps not necessary.

@peterbe

View changes

webapp-django/crashstats/crashstats/views.py
+ date_today = datetime.datetime.utcnow()
+ date_week_ago = date_today - datetime.timedelta(days=7)
+
+ context['start_date'] = date_week_ago.strftime(date_format)

This comment has been minimized.

@peterbe

peterbe Feb 19, 2014

Contributor

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.

@peterbe

peterbe Feb 19, 2014

Contributor

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.

@peterbe

This comment has been minimized.

Show comment
Hide comment
@peterbe

peterbe Feb 19, 2014

Contributor

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 :)

Contributor

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

View changes

webapp-django/crashstats/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>

This comment has been minimized.

@rhelmer

rhelmer Feb 24, 2014

Contributor

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

@rhelmer

rhelmer Feb 24, 2014

Contributor

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

ghost pushed a commit that referenced this pull request Feb 25, 2014

Merge pull request #1900 from ossreleasefeed/bug915317-display-total-…
…volume-of-gccrashes

Fix Bug 915317, display total volume of gccrashes

@ghost ghost merged commit 164049c into mozilla-services:master Feb 25, 2014

1 check passed

default Jenkins build 'socorro-github' #2849 has succeeded
Details

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment