Adding Tabular View for comparing baseline and test builds for selected platforms and metrics #131
Conversation
|
Including a screenshot of how the view looks like at the moment. |
|
@piyush286 please take a look and let me know what needs to be changed. |
|
@pinicman Yeah finally! Thanks for making all the changes suggested during our meetings. I'll try to review it later this week. |
awsafsakif
force-pushed
the
tabular
branch
2 times, most recently
from
b093248
to
12ae94c
Compare
| } | ||
| } | ||
| // Return the list of unique platforms for column generation | ||
| const uniquePlatforms = [...new Set(datas.map(item => item.buildName))]; |
There was a problem hiding this comment.
This might be a bit confusing since we are returning the buildName for platforms. The front end extracts the proper names for platforms. Even though this might reduce processing on the backend, we should still document and change uniquePlatforms to something like buildNames.
| @@ -24,7 +24,8 @@ | |||
| "react-jsx-highcharts": "^3.5.0", | |||
| "react-jsx-highstock": "^3.5.0", | |||
| "react-router": "^5.0.0", | |||
| "react-router-dom": "^5.0.0" | |||
| "react-router-dom": "^5.0.0", | |||
There was a problem hiding this comment.
We need to add package-lock.json file since we're updating this file.
| const month = (new Date().getMonth() + 1).toString(); //Current Month | ||
| const year = (new Date().getFullYear()).toString(); //Current Year | ||
|
|
||
| const jdkDate = year + ((month.length < 2) ? "0" + month : month) + ((date.length < 2) ? "0" + date : date) |
There was a problem hiding this comment.
Need to some comment here saying that we're making an assumption that the JDK data would be in the format YYYYMMDD. Same thing in dateTransform().
| const jdkDate = year + ((month.length < 2) ? "0" + month : month) + ((date.length < 2) ? "0" + date : date) | ||
|
|
||
| !('baselineJdkDate' in this.state) && (this.state.baselineJdkDate = jdkDate); | ||
| !('baselineJdkVersion' in this.state) && (this.state.baselineJdkVersion = 'O8'); |
There was a problem hiding this comment.
Again! These are our assumptions so we should list them all out along with an example.
| let column = { | ||
| Header: 'Benchmark Name', | ||
| accessor: 'benchmarkName', | ||
| Cell: props => <span>{props.value.split(",")[0]} <br/> {props.value.split(",")[1]} <br/> {props.value.split(",")[2]}</span> |
There was a problem hiding this comment.
Let's add a comment saying that we're using those 3 breaks to show benchmark name, variant and metric in different lines.
| color = '#F0F755';} | ||
| else if (val === 'N/A') { | ||
| color = 'grey';} | ||
| else if (val > 200) { |
There was a problem hiding this comment.
Different color is fine but we shouldn't show the actual value for it since it's going to be confusing since we'll be showing the relative comparison percent for most of the cells.
The relative comparison value could be more than 200% in some rare cases such as converters and DAA tests so those cells should still be considered green.
| this.setState({columns:newArray, originalColumns: newArray}); | ||
| } | ||
| // Set cell color based on comparison value | ||
| handleRegression (val) { |
There was a problem hiding this comment.
We need to handle 2 cases, which right now we are just handling as one:
- One of the builds is missing data
- Relative comparison value is way more than 100 (i.e. 200x or 1000x).
| Baseline JDK: {this.handleProp(props.value, 'baselineJdk')} <br/> | ||
| Baseline Sdk Resource: {this.handleProp(props.value, 'baselineSdkResource')} | ||
| </div> }> | ||
| <span onClick={() => this.handleLink(this.handleProp(props.value, 'buildUrl'))}> {this.handleProp(props.value, 'compare')} </span></Tooltip>; |
There was a problem hiding this comment.
Should add % to the relative comparison value to make it more clear.
| const newArray = []; | ||
| let column = { | ||
| Header: 'Benchmark Name', | ||
| accessor: 'benchmarkName', |
There was a problem hiding this comment.
We need to give it a better name. Since benchmarkName just stores the benchmark name in the database, we should replace all instance of this var with something like benchmarkNVM (i.e. benchmark name + variant + metric).
| platform = element.buildName.split("_").slice(4).join('_'); | ||
| for (const metric in element.aggregateInfo[0].metrics) { | ||
| found = false; | ||
| benchmarkName = element.aggregateInfo[0].benchmarkName + ',' + element.aggregateInfo[0].benchmarkVariant + "," + element.aggregateInfo[0].metrics[metric].name; |
There was a problem hiding this comment.
Let's use benchmarkNVM instead of benchmarkName as mentioned above.
| benchmarkName = element.aggregateInfo[0].benchmarkName + ',' + element.aggregateInfo[0].benchmarkVariant + "," + element.aggregateInfo[0].metrics[metric].name; | ||
|
|
||
| for (const currentEntry in newArray) { | ||
| // If benchmark aleady exists append to it |
| let benchmarkName = ""; | ||
| let found = false; | ||
|
|
||
| data.forEach(function (element) { |
There was a problem hiding this comment.
Maybe we should use a more meaningful name such as testResultsObject instead of element. We should do the same thing for all other loops as well.
| } | ||
|
|
||
| populateTable(data, type) { | ||
| /* Entry format, each entry in the array is an object with two fields benchmarkName and platforms |
There was a problem hiding this comment.
platforms name is confusing since it has more info beside just platform info. We should call it something like platformsSpecificData.
| /* Entry format, each entry in the array is an object with two fields benchmarkName and platforms | ||
| platforms is an object with each field being a separate platform containing the jdk data such as score, date, CI */ | ||
| const newArray = []; | ||
| let entry = {}; |
There was a problem hiding this comment.
Maybe use newObj instead of entry. We should change all other functions as well for this.
| let variant = testEntry.benchmarkName.split(",")[1]; | ||
| let metric = testEntry.benchmarkName.split(",")[2]; | ||
|
|
||
| let topLevel = {}; |
There was a problem hiding this comment.
Maybe use benchmarkLevel instead of topLevel. Same thing for midLevel and child.
|
|
||
| let topLevel = {}; | ||
| let midLevel = {}; | ||
| let child = {title: metric, value: testEntry.benchmarkName}; |
There was a problem hiding this comment.
We should mention that we are giving unique titles for these values in order to avoid showing metrics for all benchmark variants. By setting titles, we limit the display to one variant, requiring user to manually select different variants in case one wants to look up the metric for multiple variants. For example, footprint metric exists in multiple Liberty variants such as DT7, DT3 and AcmeAir.
| import benchmarkVariantsInfo from '../PerfCompare/lib/benchmarkVariantsInfo'; | ||
|
|
||
| const { Panel } = Collapse; | ||
| const { SHOW_PARENT } = TreeSelect; |
There was a problem hiding this comment.
Let's add the purpose for this.
| } | ||
| } | ||
|
|
||
| colorFilter(caller) { |
There was a problem hiding this comment.
We should explain that we have 2 filters: benchmark filter and color filter. We always call the benchmark filter first, which uses the originalData to limit to the selected benchmarks. Then we call the colorFilter, which further limits the data based on relative comparison values.
Also, we should use a better name such as firstFilter than caller since it doesn't really tell its purpose. If firstFilter is true, then we need to call the benchmark filter first. If it's false, then it means that we've already done filtering based on benchmarks, and hence, we should apply the color filter now.
| let filterValue; | ||
| // If color filter is set to ALL, do not not apply filter | ||
| if (this.state.colorFilter === "all") {return;} | ||
| else if (this.state.colorFilter === "yellow") {filterValue = 98;} |
There was a problem hiding this comment.
We should use range instead of absolute numbers.
| for (let i=0; i < this.state.consolidatedData.length; i++) { | ||
| for (let platform in this.state.consolidatedData[i].platforms) { | ||
| // Set values to N/A if they exceed the filterValue | ||
| if (parseInt(this.state.consolidatedData[i].platforms[platform].compare) > filterValue) { |
There was a problem hiding this comment.
If you choose yellow, this would also show the red cells.
| <span> Please choose the color filter: </span><select name="colorFilter" value={this.state.colorFilter} onChange={this.handleColorFilter.bind(this)}> | ||
| <option value="all">All</option> | ||
| <option value="red">Red</option> | ||
| <option value="yellow">Yellow</option> |
There was a problem hiding this comment.
Not that we care much about green, but we can just add it since it's simple.
| if (element.platforms.hasOwnProperty(platform)) { | ||
| entry.platforms[platform] = {...testEntry.platforms[platform], ...element.platforms[platform]}; | ||
| if (higherBetter) { | ||
| entry.platforms[platform].compare = Number(testEntry.platforms[platform].testScore * 100 / element.platforms[platform].baselineScore).toFixed(2); |
There was a problem hiding this comment.
If we have time, maybe we have some extra check to take care of high confidence interval.
If CI 1 + CI 2 < Regression +0.7%, then it's a confirmed regression. Else, we should show some symbol (i.e. warning sign or something) to show that confidence interval is high, and hence, we can't confirm the regression.
awsafsakif
force-pushed
the
tabular
branch
2 times, most recently
from
68ea17e
to
e56ebd9
Compare
| let color; | ||
| if (val === 0) { | ||
| color = '#ffdbac';} | ||
| else if (val <= 90) { |
There was a problem hiding this comment.
We should use both upper and lower ranges just to make it more robust. Otherwise, we're relying on the order of if else statements.
| <div className="row"> | ||
| <div className="column" style={colStyle}> SDK Resource <br/> </div> | ||
| <div className="column"> <select name="testSdkResource" className="select-css" value={this.state.testSdkResource} onChange={this.handleChange.bind(this)}> | ||
| <option value="releases">Releases</option> |
There was a problem hiding this comment.
Let's add null as well in case this isn't set.
awsafsakif
force-pushed
the
tabular
branch
3 times, most recently
from
f769e86
to
e728068
Compare
| module.exports = async ( req, res ) => { | ||
| const data = []; | ||
| const db = new TestResultsDB(); | ||
| //console.log("Request received: ", req.query.jdkVersion, req.query.jvmType, req.query.jdkDate, req.query.sdkResource); |
|
|
||
| const datas = []; | ||
| // TODO: Use available api to get build directly | ||
| const query = {buildName: {$regex: ".*_" + 'openjdk' + req.query.jdkVersion.substring(1) + "_" + req.query.jvmType + ".*perf_.*"}}; |
There was a problem hiding this comment.
Why the value is 08, 011, 012 and we try to trim it here jdkVersion.substring(1)? Can we just use the correct value?
There was a problem hiding this comment.
Yeah that's not needed. We'll fix it since jdkVersion already has the java stream info it.
|
|
||
| if (latestRun !== undefined) {datas.push( latestRun );} | ||
| } | ||
| } |
There was a problem hiding this comment.
This code is very costly. There is no need to use distinct twice. And we do not need to query one record at a time for all benchmarks x platforms and write logic to filter out data by date.
We should be able to do most of it in one query and do some light processing. We can query by the date and buildName and sort jdkBuildDateUnixTime in testData group on the buildName and benchmarkName.
Please create an issue to fix this. If you have time, please work on the issue.
| // Return the list of unique pipeline names for column generation | ||
| const buildNames = [...new Set(datas.map(item => item.buildName))]; | ||
| datas.push(buildNames); | ||
| res.send( await Promise.all(datas) ); |
There was a problem hiding this comment.
We do not need Promise.all here.
|
|
||
| this.state.inputURL[key] = value; | ||
| } | ||
| } |
There was a problem hiding this comment.
-
instead of directly setting the value in the state, please use setState()
-
We should use the existing function
getParams()and it will return an object of key and value. For example,
getParams(window.location.search)
There was a problem hiding this comment.
Nice suggestion! We'll do the same in TabularView.jsx as well. Thanks!
| <div className="column"> <select name="testJdkVersion" className="select-css" value={this.state.testJdkVersion} onChange={this.handleChange.bind(this)}> | ||
| <option value="O8">OpenJDK8</option> | ||
| <option value="O11">OpenJDK11</option> | ||
| <option value="O12">OpenJDK12</option> |
There was a problem hiding this comment.
We should not use the dropdown and hardcode jdk versions. It is a lot of work to update this for every release. An input box with a default value is a better choice. Or if you want to use dropdown, then the data should come from the database.
Same for the rest of dropdown. And each of them should be a react component.
If you do not have time to fix it now, please open an issue for it.
There was a problem hiding this comment.
Yeah this one definitely makes sense. Else, we'll have to update these options whenever there's a new java version or type. Will soft-code these options and get it from the database.
There was a problem hiding this comment.
@llxia Soft-coding these options would add more distinct in the code. Are you fine with the more additions or do we leave this hard-coded for now? Thanks!
There was a problem hiding this comment.
Nvm! We found a more efficient way to do it by grouping with $addToSet, so we are soft-coding all those options.
|
Please format the code. Thanks. |
@llxia Could you please clarify how can we get the distinct platforms, benchmark names and others such as Java version, type, SDK Resource (once we soft-code by querying from the database) without using
I don't think we can query by a specific date. We want to get the latest JDK build that was built on or "before" the selected JDK date so we can't really query for one particular date. We use the Since we care about the latest run for each benchmark and platform, I'm not sure how we can query in any other way. Sorry, I'm not getting your point. If that's the case, then could you please put the pseudo step-by-step code for it? We could also have a quick meeting if that's more convenient for you. |
|
We'll be adding another dropdown menu for build server so that we can compare builds only from one specific server since it wouldn't make sense to compare the data that would have run on different machines under different servers. Since we already store the server info under |
I just think there should be a more efficient way of doing the query. I would like to have an issue open so we can revisit this. |
|
@llxia Sounds good, Lan! Opened an issue for it: #133. Adding the dropdown option for server (i.e. |
…ed platforms and metrics - Closes AdoptOpenJDK#37 - Current filters: benchmark, platform, cell color - Allows comparison between different jdk versions and types - Cell on click redirects to Perf Compare - Perf Compare changed to fill in values from URL on load - Added sdkResource to parser, will be added as field to database - Warning sign appears if total CI exceeds percentage difference Co-Authored-By: Piyush Gupta <piyush286@gmail.com> Signed-off-by: Awsaf Arefin Sakif <awsaf.sakif@ibm.com>
|
@llxia Could you please take a final look whenever you get a chance? Thanks! |
|
Please update the code on the internal server. And we should enable the pref builds and ensure these builds are monitored by TRSS. Thanks. |
Co-authored-by: Piyush Gupta piyush286@gmail.com
Signed-off-by: Awsaf Arefin Sakif awsaf.sakif@ibm.com