705293 - Y axis should be adjusted according to timeline you are looking at

Status: RESOLVED DUPLICATE of bug 641413

(Webtools Graveyard :: Graph Server, defect)

Description

[Armen [:armenzg]]

For example, if you want to look at "FennecBench Pan" for "mobile" and "mozilla-inbound" you get this:
http://graphs-new.mozilla.org/graph.html#tests=[[69,11,20],[69,63,20]]&sel=none&displayrange=7&datatype=running

If you notice the Y axis starts from 0 to somewhere past a 1000.
This makes it very hard to tell if there are regressions since the scoring goes between 100 and 150. In fact, there is a regression for this example but can barely be noticed visually.

The reason this Y axis range is so vast can be seen if we load up the 365 days view:
http://graphs-new.mozilla.org/graph.html#tests=[[69,11,20],[69,63,20]]&sel=none&displayrange=365&datatype=running

We can see that back in January we scored 1012 and that seems to be taken into consideration for the 7 days view.

Fixing this would be extremely valuable/useful as I constantly try to hunt regressions.

Comment 1

[Robert Helmer [:rhelmer]]

I think this would be pretty easy to do, iirc we're loading the whole dataset and then relying on flot to restrict the date range. It may be possible to do this with flot configuration, or we may need to just give it the data only for the right range and then replot.

Comment 2

[Robert Helmer [:rhelmer]]

Attachment: only render points for current day view

Since we're iterating over the total list of points anyway (to convert from graphserver format to flot), let's throw away the ones we don't need. I did this rather quickly and haven't thought about making it more succinct, I think it's pretty readable at least.

A couple concerns I have:

I considered not bothering figuring out minV/maxV but the way it sets a margin is kind of nice. However reading the flot API docs it looks like it supports this directly, so that might not be necessary actually.

One thing this patch makes more obvious is that 0 is always visible on the x axis, so results tend to cluster towards the top of the chart - this was at sayrer's request but I have had complaints about that, perhaps we should just revert it as part of this patch and let flot auto-scale the x axis.

Comment 3

[Robert Helmer [:rhelmer]]

Comment on attachment 608956 [details] [diff] [review]
only render points for current day view

Hey jeads, feedback would be greatly appreciated if you have any - see comment 2 for more info.

Comment 4

[Robert Helmer [:rhelmer]]

Attachment: only render points for current day view, v2

The patch from comment 2, plus removing all the magick around minV/maxV and forcing yaxis:0

The more I look at this, the more I think flot could just be handling this bit. I think we need to keep minT/maxT in place for the current zooming scheme to work though.

Comment 5

[Ryosuke Niwa]

Comment on attachment 608958 [details] [diff] [review]
only render points for current day view, v2

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

Looks great to me. I wanted to do this for a while :)

::: js/common.js
@@ +620,2 @@
>      var count = 0;
> +    var displayDays = GraphCommon.displayDaysInMicroseconds();

Why do we need to move this statement? It seems unnecessary.

Comment 6

[Robert Helmer [:rhelmer]]

(In reply to Ryosuke Niwa from comment #5)
> Comment on attachment 608958 [details] [diff] [review]
> only render points for current day view, v2
> 
> Review of attachment 608958 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great to me. I wanted to do this for a while :)

Cool, thanks for the review.
 
> ::: js/common.js
> @@ +620,2 @@
> >      var count = 0;
> > +    var displayDays = GraphCommon.displayDaysInMicroseconds();
> 
> Why do we need to move this statement? It seems unnecessary.

This was leftover from an approach I abandoned, however looking at it now the definition is happening in the $.each() so it's probably better off where it is now (displayDays is only used once so it might be better to just avoid this assignment, but I think it makes the code look just a little nicer).

Comment 7

[Robert Helmer [:rhelmer]]

Went ahead and pushed:
http://hg.mozilla.org/graphs/rev/65dca7667fcf

This is a kind of a big change, so I wanted to get it on stage for testing ASAP:
http://graphs.allizom.org

The graphs are a lot more lively looking now! Could definitely use help testing this :)

Comment 8

[Robert Helmer [:rhelmer]]

Had to make a few small changes to get the static graphs working (it was assuming that the series was complete and using the minT/maxT from that instead of calculating it based on the displayDays setting):

http://hg.mozilla.org/graphs/rev/67d7b13532a8
http://hg.mozilla.org/graphs/rev/f3b4d0745e4a

These are now live on http://graphs.allizom.org

Comment 9

[Jonathan Eads ( :jeads )]

(In reply to Robert Helmer [:rhelmer] from comment #3)
> Comment on attachment 608956 [details] [diff] [review]
> only render points for current day view
> 
> Hey jeads, feedback would be greatly appreciated if you have any - see
> comment 2 for more info.

Sorry for the delayed response.  In general, letting flot do the auto-scaling is a good idea, simplifies things.  

One thing I'm noticing on http://graphs.allizom.org, the y-scale range sometimes varies significantly from the pre-computed Dashboard chart to the detail chart.  The x-axis range is the same between the Dashboard and Detail views for the 7 day range, I was thinking they should yield the same y-axis range but in some cases they do not.  Is the min/max still being chosen from a larger x-range on the Dashboard charts?

Dashboard: SunSpider/Windows XP y-axis: 15-30
Detail:http://graphs.allizom.org/graph.html#tests=[[162,1,1]]&sel=none&displayrange=7&datatype=running , y-axis: 16.35-16.95

The values for SunSpider/Windows XP from March 21-27 are all essentially the same but the spread on the detail chart gives the impression of a lot of variation, if you don't inspect the y-axis range carefully it might be misleading.

I think there is a chart option in flot called autoscaleMargin that is "the fraction of margin
that the scaling algorithm will add to avoid that the outermost points ends up on the grid border"

{ yaxis: { autoscaleMargin:0.02 } }, 0.02 is the default value, if you increase it a bit I think that will put some buffer between the data min/max and the y-axis min/max.  Maybe add this to PLOT_OPTIONS/OVERVIEW_OPTIONS?

Comment 10

[Robert Helmer [:rhelmer]]

(In reply to Jonathan Eads ( :jeads ) from comment #9)
> (In reply to Robert Helmer [:rhelmer] from comment #3)
> > Comment on attachment 608956 [details] [diff] [review]
> > only render points for current day view
> > 
> > Hey jeads, feedback would be greatly appreciated if you have any - see
> > comment 2 for more info.
> 
> Sorry for the delayed response.  In general, letting flot do the
> auto-scaling is a good idea, simplifies things.  
> 
> One thing I'm noticing on http://graphs.allizom.org, the y-scale range
> sometimes varies significantly from the pre-computed Dashboard chart to the
> detail chart.  The x-axis range is the same between the Dashboard and Detail
> views for the 7 day range, I was thinking they should yield the same y-axis
> range but in some cases they do not.  Is the min/max still being chosen from
> a larger x-range on the Dashboard charts?
> 
> Dashboard: SunSpider/Windows XP y-axis: 15-30
> Detail:http://graphs.allizom.org/graph.html#tests=[[162,1,
> 1]]&sel=none&displayrange=7&datatype=running , y-axis: 16.35-16.95

Hmm looking at it right now, they look exactly the same to me?
http://graphs.allizom.org/images/dashboard/flot-162-1-1_7.png
http://graphs.allizom.org/graph.html#tests=[[162,1,1]]&sel=none&displayrange=7&datatype=running

We do use slightly different flot settings between the dashboard images and the custom chart so I do believe that it might have auto-scaled a little differently between the two screens, we should be able to bring these closer together though. I'll look into this.

I had misread your comment - there does seem to be a different bug, which is a mismatch for total amount of data between the dashboard and the custom view, when looking beyond 30 days:

http://graphs.allizom.org/graph.html#tests=[[83,1,12]]&sel=none&displayrange=365&datatype=running
http://graphs.allizom.org/graph.html#tests=[[83,1,12]]&sel=none&displayrange=365&datatype=running

This is probably a bug in the dashboard code related to the new changes.

 
> The values for SunSpider/Windows XP from March 21-27 are all essentially the
> same but the spread on the detail chart gives the impression of a lot of
> variation, if you don't inspect the y-axis range carefully it might be
> misleading.
> 
> I think there is a chart option in flot called autoscaleMargin that is "the
> fraction of margin
> that the scaling algorithm will add to avoid that the outermost points ends
> up on the grid border"
> 
> { yaxis: { autoscaleMargin:0.02 } }, 0.02 is the default value, if you
> increase it a bit I think that will put some buffer between the data min/max
> and the y-axis min/max.  Maybe add this to PLOT_OPTIONS/OVERVIEW_OPTIONS?


I'll give this a try, thanks!

Comment 11

[Ryosuke Niwa]

On somewhat tangential note, can we add some UI to adjust Y axis manually? e.g. ctrl and "+"/"-" or wheel scrolling? A lot of people have complained to me about it.

Comment 13

[Robert Helmer [:rhelmer]]

(In reply to Ryosuke Niwa from comment #11)
> On somewhat tangential note, can we add some UI to adjust Y axis manually?
> e.g. ctrl and "+"/"-" or wheel scrolling? A lot of people have complained to
> me about it.

There's bug 641413 - would you mind following up there?

Comment 14

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #10)
> (In reply to Jonathan Eads ( :jeads ) from comment #9)
> > (In reply to Robert Helmer [:rhelmer] from comment #3)
> > > Comment on attachment 608956 [details] [diff] [review]
> > > only render points for current day view
> > > 
> > > Hey jeads, feedback would be greatly appreciated if you have any - see
> > > comment 2 for more info.
> > 
> > Sorry for the delayed response.  In general, letting flot do the
> > auto-scaling is a good idea, simplifies things.  
> > 
> > One thing I'm noticing on http://graphs.allizom.org, the y-scale range
> > sometimes varies significantly from the pre-computed Dashboard chart to the
> > detail chart.  The x-axis range is the same between the Dashboard and Detail
> > views for the 7 day range, I was thinking they should yield the same y-axis
> > range but in some cases they do not.  Is the min/max still being chosen from
> > a larger x-range on the Dashboard charts?
> > 
> > Dashboard: SunSpider/Windows XP y-axis: 15-30
> > Detail:http://graphs.allizom.org/graph.html#tests=[[162,1,
> > 1]]&sel=none&displayrange=7&datatype=running , y-axis: 16.35-16.95
> 
> Hmm looking at it right now, they look exactly the same to me?
> http://graphs.allizom.org/images/dashboard/flot-162-1-1_7.png
> http://graphs.allizom.org/graph.html#tests=[[162,1,
> 1]]&sel=none&displayrange=7&datatype=running
> 
> We do use slightly different flot settings between the dashboard images and
> the custom chart so I do believe that it might have auto-scaled a little
> differently between the two screens, we should be able to bring these closer
> together though. I'll look into this.

Ah ok here's an example of what you're talking about:

http://graphs.allizom.org/images/dashboard/flot-83-1-12_7.png
http://graphs.allizom.org/graph.html#tests=[[83,1,12]]&sel=none&displayrange=7&datatype=running

x axis range is 500-600 on dashboard, but 520-560 on custom view.

I suspect that flot is doing different things here because the resolution of the graph is different.

Comment 16

[Robert Helmer [:rhelmer]]

BTW so this is pretty much ready, there is a regression involving the dashboard graphs that I haven't had time to nail down. Will work on it this week.

Comment 17

[Geoff Brown [:gbrown]]

Any update on this? I talked to :rhelmer on irc a couple weeks ago and thought that a fix was imminent, but many of the android talos graphs are still severely affected. eg Android rck/tcheckerboard, http://graphs.mozilla.org/graph.html#tests=[[175,11,20]]&sel=none&displayrange=7&datatype=running, where the y-axis range is to 30000+, but data values are typically around 120.

Comment 18

[Robert Helmer [:rhelmer]]

(In reply to Geoff Brown [:gbrown] from comment #17)
> Any update on this? I talked to :rhelmer on irc a couple weeks ago and
> thought that a fix was imminent, but many of the android talos graphs are
> still severely affected. eg Android rck/tcheckerboard,
> http://graphs.mozilla.org/graph.html#tests=[[175,11,
> 20]]&sel=none&displayrange=7&datatype=running, where the y-axis range is to
> 30000+, but data values are typically around 120.

This is on me; I have some unrelated work to do by the end of the quarter.

The code is landed on default branch right now, the only problem is that the (node.js) script that generates the dashboard images seems to not be working correctly with this change, if anyone wants to take a crack at it in the meantime.

Comment 19

[Robert Helmer [:rhelmer]]

OK this feature is passing all the local testing I have put it through, I need to get the data on staging refreshed and announce that this change is coming before we push to prod.

Comment 20

[Armen [:armenzg]]

Anywhere I can look at on staging?

Comment 21

[Robert Helmer [:rhelmer]]

(In reply to Armen Zambrano G. [:armenzg] from comment #20)
> Anywhere I can look at on staging?

Yes once bug 795949 is resolved, we should be able to test on http://graphs.allizom.org

Thanks!

Comment 22

[Robert Helmer [:rhelmer]]

Hmm so I see the problem on staging still:

http://graphs.allizom.org/images/dashboard/flot-206-1-12_90.png
http://graphs.allizom.org/graph.html#tests=[[206,1,12]]&sel=none&displayrange=90&datatype=running

I can reproduce it locally, and surprisingly it goes away if I remove xaxis:{mode:'time'} (which stops treating it as time-series data)... this makes me wonder if there's some problem with parsing the date?

Not sure why it works for the custom graph+overview, that code had a lot of changes related to the yaxis-zooming feature so it might be related to that. I'll dig in a bit tomorrow.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Any progress on this bug? It would significantly help to combat the unreliability of the automated regression detection.

Comment 24

[Geoff Brown [:gbrown]]

I noticed a huge improvement in the graphs just recently, at least those for Android, like http://graphs.mozilla.org/graph.html#tests=[[16,11,20]]&sel=none&displayrange=7&datatype=running -- looks great now.