Replace react-vis with Recharts #2242
Conversation
|
Tests in ScatterPlot are failing because the behavior of the plot changed, see screenshots. I'm new to jaeger so I don't know how how much I should change this behavior to resemble the old one. What do you think? This is the new recharts version: This is the version from docker-all-in-one: |
|
Please make sure your new commits are signed. See CONTRIBUTING. |
Signed-off-by: Mark Pollmann <[email protected]>
There was a problem hiding this comment.
the behavior of the plot changed
What specific changes are you not sure about? The new chart looks good. Could use a bit smaller font. I like that the axis labels are now outside. But it's not possible to tell if the dynamic behavior changed, like the crosshair, point data tooltip, etc.
Suggestion: change y-axis to "Trace Duration", to be explicit.
| @@ -130,8 +130,10 @@ const calcDisplayTimeUnit = (serviceLatencies: ServiceMetricsObject | ServiceMet | |||
| }; | |||
|
|
|||
| // export for tests | |||
| export const yAxisTickFormat = (timeInMS: number, displayTimeUnit: string) => | |||
| convertToTimeUnit(timeInMS * 1000, displayTimeUnit); | |||
| export const yAxisTickFormat = (timeInMS: number, displayTimeUnit: string) => { | |||
There was a problem hiding this comment.
| export const yAxisTickFormat = (timeInMS: number, displayTimeUnit: string) => { | |
| export const yAxisTickFormat = (timeInMS: number, displayTimeUnit: string): string => { |
?
| export const yAxisTickFormat = (timeInMS: number, displayTimeUnit: string) => | ||
| convertToTimeUnit(timeInMS * 1000, displayTimeUnit); | ||
| export const yAxisTickFormat = (timeInMS: number, displayTimeUnit: string) => { | ||
| const formattedValue = convertToTimeUnit(timeInMS * 1000, displayTimeUnit); |
There was a problem hiding this comment.
nit
| const formattedValue = convertToTimeUnit(timeInMS * 1000, displayTimeUnit); | |
| const scaledValue = convertToTimeUnit(timeInMS * 1000, displayTimeUnit); |
| @@ -20,23 +20,13 @@ exports[`<OperationsGraph> Graph rendered successfully 1`] = ` | |||
| <div | |||
| className="ops-container" | |||
| > | |||
| <XYPlot | |||
| className="" | |||
| <ForwardRef | |||
There was a problem hiding this comment.
ideally we should get rid of snapshot testing here, such large snapshot is useless and highly dependent on implementation details of the lib
| <Area type="monotone" dataKey="y" stroke={color} fill={color} {...dynProps} /> | ||
| <Line type="monotone" dataKey="y" stroke={color} {...dynProps} /> |
There was a problem hiding this comment.
would be nice to comment what these two lines do
| }); | ||
|
|
||
| it('Crosshair map test', () => { | ||
| it('Tooltip test', () => { |
There was a problem hiding this comment.
This changes what the test was looking for. Is the crosshair pointer not available? Or we can't test for it? I assume it's just part of the style.
| `label: ${exampleEntry.payload.label * 100}`, | ||
| `x: ${formattedLabel}`, | ||
| `Y: ${exampleEntry.payload.y}`, |
There was a problem hiding this comment.
nit: this is testing production code with production code (akin to assert f(x) == f(x)). I would rather have precise strings in the expected dict - makes for a simpler and easier to read test, which is not tautological.
| expect(markSeries.length).toEqual(1); | ||
|
|
||
| const circle = wrapper.find('.rv-xy-plot__series--mark circle:first-child'); | ||
| const circle = wrapper.find(Tooltip); |
| expect(circle.length).toEqual(1); | ||
|
|
||
| circle.simulate('mouseOver'); | ||
| const hint = wrapper.find(Hint); | ||
| const hint = wrapper.find(Tooltip); |
| @@ -191,7 +191,7 @@ export class UnconnectedSearchResults extends React.PureComponent<SearchResultsP | |||
| x: t.startTime, | |||
| y: t.duration, | |||
| traceID: t.traceID, | |||
| size: t.spans.length, | |||
| size: t.spans.length * 30, | |||
There was a problem hiding this comment.
what is the motivation for *30?
|
Thanks for the review, @yurishkuro! Will try to finish it by Friday.
Mostly that the x-axis is no longer uniform in time. If you look at the screenshot of the docker version, every tick is around 17 minutes. In the recharts version the first tick is 2min, the next one is 45min. It scales automatically to 'un-bunch' the circles (pretty cool, actually). |
oh, wow, that's terrible. It only makes sense to do if it displayed some visual indicator of timeline tear, kind of like what GitHub is doing: Can we set some option to use linear scale? |
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test