Skip to content
This repository has been archived by the owner on Nov 28, 2020. It is now read-only.

Benchmakr charts Y-axis values offset #72

Closed
mhdawson opened this issue Nov 15, 2016 · 6 comments
Closed

Benchmakr charts Y-axis values offset #72

mhdawson opened this issue Nov 15, 2016 · 6 comments

Comments

@mhdawson
Copy link
Member

Just noticed something strange. Even though the relative relationships between releases make sense in the charts, it looks to me like the x-axis with the value on a given day is off versus what is reported in the benchmark jobs.

May be an issue with the underlying chart generation program, will need to investigate to see what is going on.

@CurryKitten
Copy link
Contributor

I've been looking into this, and as an aside point, the instructions about installing Chart.js and Chart.Scatter.js (https://github.com/nodejs/benchmarking/tree/master/tools/chartGen/lib) would benefit by being more specific on versions needed. I found that the lastest versions of the libraries weren't compatible with the chart get code.

@mhdawson was able to provide me a copy of the versions running in CI (Chart.js is at 1.0.2, latest is 2.4.0) and a copy of the results database. My findings so far is that the html that's generated looks correct when viewed in a browser. The problem seems to occur when phantomjs is used to create the png snapshot. This rendering misaligns the data against the Y axis. I'm trying to determine what the cause of this is.

@CurryKitten
Copy link
Contributor

The problem was in because the data on the charts animates into view. There was already a 1 second delay for the page to complete before the screen shot was taken, but this wasn't enough to finish the animation. I've just added a PR to turn the animation off - I've tested this and the generated png files look the same as the actual html

@mhdawson
Copy link
Member Author

PR for fix went in, had validated one chart so expect it will resolve. Will validate/close this issue once charts are next fully transferred to benchmarking.nodejs.org.

@mhdawson
Copy link
Member Author

@CurryKitten can you also submit a PR to update the instructions to specify the versions needed so that we can recreate in case of machine loss.

@CurryKitten
Copy link
Contributor

@mhdawson Yes - I'll double check those versions and add an appropriate PR

@mhdawson
Copy link
Member Author

Ok, sniff checked acemair latency and value in job from last night matches what we get on the chart in respect to the Y axis in chart on benchmarking.nodejs.org. closing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants