Repository navigation
chart: fix division by zero and step overflow in json engine - #2011
brharrington merged 2 commits into
Conversation
In JsonGraphEngine, graphDatapoints was calculated by dividing (config.step / 1000). For sub-second step sizes (such as 500ms or 100ms, supported in Step.allowedStepSizes), integer division evaluates to 0 and throws ArithmeticException: / by zero. Using config.step directly in milliseconds avoids division by zero while preserving the exact datapoint count. Additionally, step was cast to Int, which overflows for large step intervals (> 24.8 days) and causes timestamp += step to decrease in an infinite loop. Keep step as Long. Also add unit test coverage for stats.json and ensure test string stripping is newline-agnostic. Signed-off-by: reginaldalfret <[email protected]>
| val end = config.endTime.toEpochMilli | ||
| val graphLines = config.plots.map(_.data.size).sum | ||
| val graphDatapoints = graphLines * ((end - start) / (config.step / 1000) + 1) | ||
| val graphDatapoints = graphLines * ((end - start) / config.step + 1) |
There was a problem hiding this comment.
endTime is exclusive and the values loop above emits (end - start) / step rows, so the + 1 overcounts by one per line. For example, the 3 minute window in the new test emits 3 rows but reports graphDatapoints = 4.
Suggest moving this into GraphDef, something like:
def numDatapoints: Long = (endTime.toEpochMilli - startTime.toEpochMilli) / stepand using it from both JsonGraphEngine and DefaultGraphEngine so they stay consistent.
| val end = config.endTime.toEpochMilli / 1000 | ||
| val start = config.startTime.toEpochMilli | ||
| val end = config.endTime.toEpochMilli | ||
| val graphLines = config.plots.map(_.data.size).sum |
There was a problem hiding this comment.
plots.map(_.data.size) counts all data defs including spans and messages, but the output only includes lines. A graph with one line and a :vspan reports graphLines = 2. seriesList.size would match what is actually written.
| @@ -74,10 +74,10 @@ class JsonGraphEngine extends GraphEngine { | |||
| gen.writeEndArray() | |||
|
|
|||
| if (config.stats.inputLines > 0) { | |||
There was a problem hiding this comment.
Minor, not from this PR: DefaultGraphEngine also checks config.loadTime > 0. Without that, this can emit "dataFetchTime": -1 when loadTime is left at the default.
| |"metrics":[{"name":"42.0"}], | ||
| |"values":[[42.0],[42.0],[42.0]], | ||
| |"notices":[], | ||
| |"explain":{"dataFetchTime":123,"inputLines":10,"intermediateLines":20,"graphLines":1,"inputDatapoints":5,"intermediateDatapoints":10,"graphDatapoints":4} |
There was a problem hiding this comment.
This expects graphDatapoints = 4, but values has 3 rows. Same for the sub-second test below (2 rows, expects 3). Should be updated along with the off-by-one fix.
| process(new StatsJsonGraphEngine, expected) | ||
| } | ||
|
|
||
| test("json with explain stats") { |
There was a problem hiding this comment.
Nit: the new tests repeat the same build/write/compare block. Consider an overload of process that takes a GraphDef.
| |2012-01-01T00:00:00Z,42.000000,NaN | ||
| |2012-01-01T00:01:00Z,42.000000,NaN | ||
| |""".stripMargin | ||
| |""".stripMargin.replace("\r\n", "\n") |
There was a problem hiding this comment.
The CRLF handling seems unrelated to the fix and only covers a few suites, many others compare against stripMargin literals in the same way. If this is for Windows checkouts, a .gitattributes with *.scala text eol=lf would handle it repo wide. I'd suggest dropping these changes from this PR.
- Add GraphDef.numDatapoints and use it from both the JSON and PNG engines. End time is exclusive so the count no longer adds one, and the PNG engine no longer divides millis by seconds or fails for sub-second steps. - Require step > 0 on GraphDef. - Only count lines, not spans or messages, for graphLines. - Only emit the JSON explain block when loadTime is known, consistent with the PNG engine. - Bound test output so a non-terminating values loop fails fast and share the render helper across tests. - Drop the CRLF test changes, unrelated to this fix. - Update PNG golden images for the corrected datapoint count.
Summary
This PR addresses two edge-case issues in JsonGraphEngine:
Verification
All 335 tests in atlas-chart pass: