Remove dependencies on commons-math3 and ssj by implementing simple linear regression directly, replacing the build duration distribution chart with a histogram, and deleting the smoothed trend lines in the test history charts
#639
Conversation
| durationSeries.put("symbolSize", "0"); | ||
| durationSeries.put("sampling", "lttb"); | ||
| durationSeries.put("type", "bar"); | ||
| durationSeries.put("barWidth", "99%"); |
There was a problem hiding this comment.
Slightly less than 100% so you see a border between each bar.
| durationSeries.put("symbolSize", "0"); | ||
| durationSeries.put("sampling", "lttb"); | ||
| durationSeries.put("type", "bar"); | ||
| durationSeries.put("barWidth", "99%"); | ||
| ArrayNode durationData = MAPPER.createArrayNode(); | ||
| durationSeries.set("data", durationData); | ||
| ObjectNode durationStyle = MAPPER.createObjectNode(); | ||
| durationSeries.set("itemStyle", durationStyle); | ||
| durationStyle.put("color", "--success-color"); // "rgba(50, 200, 50, 0.8)"); |
There was a problem hiding this comment.
This color is a bit ugly now that it is not just a simple line in case anyone has any recommendations. I think ideally this would be a stacked histogram chart with each build result shown in a different color.
| int idx2 = Math.max(0, Math.min(idx, lrY.length - 1)); | ||
| lrY[idx2]++; | ||
| } | ||
| for (int i = 0; i < lrY.length; ++i) { | ||
| lrX[i] = ((minDuration + (maxDuration - minDuration) / lrY.length * i) / scale * 100.0); | ||
| lrX[i] = minDuration + step * (i + 0.5); |
There was a problem hiding this comment.
Offset the X values so they are in the middle of the bucket. We are no longer scaling them into the range 0-1 because we are using them directly in our data series.
| // Use float for smaller JSONs. | ||
| domainAxisLabels.add((float) (Math.round(mul * z * roundMul) / roundMul)); | ||
| for (int i = 0; i < lrY.length; i++) { | ||
| double scaledX = mul * lrX[i]; |
There was a problem hiding this comment.
We no longer round our X values in the backend (z in the old code), because our axis type is now value instead of category, and so if we round here our bars will have weird gaps and not be contiguous.
| domainAxisLabels.add((float) (Math.round(mul * z * roundMul) / roundMul)); | ||
| for (int i = 0; i < lrY.length; i++) { | ||
| double scaledX = mul * lrX[i]; | ||
| double y = lrY[i]; |
There was a problem hiding this comment.
Nothing fancy with the Y values anymore, this is just the frequency of build durations in this bucket. We can probably make lrY an int[] now.
| formatter: function(value) { | ||
| return Math.round(value * model.distribution.xAxis.roundingFactor) / model.distribution.xAxis.roundingFactor; |
There was a problem hiding this comment.
Round the labels in the frontend.
| data: model.distribution.domainAxisLabels, | ||
| min: model.distribution.xAxis.min, | ||
| max: model.distribution.xAxis.max, | ||
| minInterval: model.distribution.xAxis.interval, |
There was a problem hiding this comment.
What I really wanted was to be able to tell echarts to pick an interval that is a multiple of model.distribution.xAxis.interval, and then let it automatically choose which multiple based on screen and label size, but I didn't see any way to do it.
If you set interval: model.distribution.xAxis.interval, then you get one grid line per bucket, which is nice, but the labels are unreadably close to each other and I couldn't find a way to fix that dynamically. You can set interval: model.distribution.xAxis.interval * 2 and things look fine on "normalish" laptop screen size, but again it doesn't scale at all if you shrink your screen size.
I also wanted to have dynamic minorTick support, e.g. if we let echarts pick the interval, could we at least set up the minor ticks to align with the buckets? Unfortunately I don't think so, you can only control how many minor ticks you want between each major tick, e.g. if you specify interval: model.distribution.xAxis.interval * 3, you could specify minorTick { show: true, splitNumber: 3 }, but again that doesn't adjust based on the data or screen size at all.
This is a followup to #638 and #625 to further simplify the dependencies after I discussed it with some colleagues. This plugin is used pretty widely in the Jenkins ecosystem, so it seems worth trying to minimize its dependencies, especially to eliminate dependencies which are not being actively maintained.
This PR does 3 things (you can review commit by commit if you like):
commons-math3dependency by just directly implementing a simple linear regression algorithm. This is very straightforward since we are only using it for a trend line and don't care about any other data related to the regression.SmoothingCubicSpline-based build duration distribution chart with a histogram, which I think is a better representation of the data anyway.SmoothingCubicSpline-based "Smooth of X" trend lines that are only shown in the test history widget if more than 200 builds' worth of test results are available. I did this so we could get rid of thessjdependency completely. Here is my justification:SmoothingCubicSplinein an actively-maintained library, and I am not confident in being able to reimplement it from scratch correctly in a reasonable amount of timessjis not actively maintained and has some warning signs, for example its 3.3.2 release is not listed anywhere in publicly available Git commit historySmoothingCubicSplinein an actively-maintained library with minimal dependencies and that ideally is not pulling in a whole grab bag of unrelated functionality that we do not needSmoothingCubicSplineinto this repository while still having license information be reported accurately in Jenkins on the/manage/about/pageHere are screenshots with and without the PR:
With this PR:
Without this PR:
Testing done
Submitter checklist