Replace react-vis with Recharts #1722

anikdhabal · 2023-08-22T17:44:04Z

Which problem is this PR solving?

Resolves #1597

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: yarn lint and yarn test

Signed-off-by: Anik Dhabal Babu <[email protected]>

yurishkuro

@anshgoyalevil mind reviewing this?

packages/jaeger-ui/src/components/Monitor/ServicesView/index.test.js

anshgoyalevil · 2023-08-22T18:56:30Z

On it 🚀

package.json

packages/jaeger-ui/src/components/Monitor/ServicesView/index.tsx

packages/jaeger-ui/src/components/Monitor/ServicesView/operationDetailsTable/opsGraph.tsx

anshgoyalevil · 2023-08-22T19:21:14Z

packages/jaeger-ui/src/components/Monitor/ServicesView/serviceGraph.test.js

-      crosshairValues: [
-        {
-          key: 1,
-          ...serviceMetrics.service_call_rate.metricPoints[0],
-        },
-      ],
-    });
-    expect(wrapper.find('AreaSeries').render()).toMatchSnapshot();
-    expect(wrapper.find('LineSeries').render()).toMatchSnapshot();
-  });
-
-  it('AreaSeries onNearestX, XYPlot onMouseLeave', () => {
-    wrapper.setProps({
-      ...props,
-      loading: false,
-      metricsData: serviceMetrics.service_call_rate,
    });
-    wrapper.find('AreaSeries').at(0).prop('onNearestX')({ x: 1, y: 2 }, { index: 7 });
-    expect(wrapper.state().crosshairValues).toEqual([{ label: 0.95 }]);
-    wrapper.find('XYPlot').prop('onMouseLeave')();
-    expect(wrapper.state().crosshairValues).toEqual([]);


I think removing these tests defeats the purpose of test-driven development. Can we add some similar assumptions for tooltip hover test to verify the functionality?

Yes, on it.

anshgoyalevil · 2023-08-22T19:25:01Z

packages/jaeger-ui/src/components/Monitor/ServicesView/serviceGraph.tsx

-  yAxisTickFormat?: (v: number) => number;
+  yAxisTickFormat?: (v: number) => string;


Can't we return a number here?

Nope, as I previously described that timeFormatter function only accept string.

anshgoyalevil · 2023-08-22T19:30:27Z

packages/jaeger-ui/src/components/Monitor/ServicesView/serviceGraph.tsx

+            type="monotone"
+            dataKey="y"
+            connectNulls
+            fillOpacity={0.1}
+            stroke={color || this.colors[idx]}
+            fill={color || this.colors[idx]}


Aren't we passing on the data prop here?

There are no props to pass the data here. Also, we don't need to pass the data props here; the data props is passed to the parent component.

anshgoyalevil · 2023-08-22T19:30:47Z

packages/jaeger-ui/src/components/Monitor/ServicesView/serviceGraph.tsx

-            data={line.metricPoints ? line.metricPoints : []}
-            color={[color || this.colors[idx]]}
-          />
+          <Line connectNulls key={i++} type="monotone" dataKey="y" stroke={color || this.colors[idx]} />


Aren't we passing on the data prop here?

packages/jaeger-ui/src/components/Monitor/ServicesView/serviceGraph.tsx

Signed-off-by: Anik Dhabal Babu <[email protected]>

yurishkuro · 2023-08-27T17:43:02Z

packages/jaeger-ui/src/components/Monitor/ServicesView/serviceGraph.test.js

-  it('Loading indicator is displayed', () => {
-    expect(wrapper).toMatchSnapshot();
+  it('displays loading indicator when loading', () => {
+    expect(wrapper.find('LoadingIndicator').exists()).toBe(true);


exists()).toBe(true) - is there a simpler form like isNotNull?

I don't think. But there are similar from like toBeTruthy() or toBeTrue()

jaeger-ui/packages/jaeger-ui/src/components/Monitor/ServicesView/index.test.js

Line 289 in 60093a4

expect(wrapper.find(ServiceGraph).first().prop('error')).not.toBeNull();

yurishkuro · 2023-08-27T17:46:47Z

Q: in the screenshot all dots are the same size, but the original chart used number of spans to control the size of the trace. Is such functionality supported in recharts?

anikdhabal · 2023-08-27T20:33:32Z

Q: in the screenshot all dots are the same size, but the original chart used number of spans to control the size of the trace. Is such functionality supported in recharts?

Apart from the X and Y axes, Recharts also provides a Z-axis. With the assistance of the range property of Z axis, we can achieve this desired functionality. (similar to sizeRange in react-vis)

yurishkuro · 2023-08-27T22:51:52Z

Apart from the X and Y axes, Recharts also provides a Z-axis. With the assistance of the range property of Z axis, we can achieve this desired functionality. (similar to sizeRange in react-vis)

Are you planning to add that? Otherwise you're removing exiting functionality

Signed-off-by: Anik Dhabal Babu <[email protected]>

anikdhabal · 2023-08-29T21:26:49Z

Apart from the X and Y axes, Recharts also provides a Z-axis. With the assistance of the range property of Z axis, we can achieve this desired functionality. (similar to sizeRange in react-vis)

Are you planning to add that? Otherwise you're removing exiting functionality

I've added Z-axis for the size range and set the data key to 'size' as it carries the span length.

yurishkuro · 2023-08-29T21:29:04Z

when I run yarn test locally I am getting various errors related to your changes, like this:

 FAIL  src/components/SearchTracePage/SearchResults/ScatterPlot.test.js
  ● Console

    console.error
      Warning: Failed prop type: Invalid prop `data[0].traceID` of type `number` supplied to `ScatterPlot`, expected `string`.
          at data (/Users/ysh/dev/jaegertracing/jaeger-ui/packages/jaeger-ui/src/components/SearchTracePage/SearchResults/ScatterPlot.jsx:33:11)

      55 | it('<ScatterPlot /> should render base case correctly', () => {
      56 |   const wrapper = shallow(
    > 57 |     <ScatterPlot
         |     ^
      58 |       data={[
      59 |         { x: Date.now() - 3000, y: 1, traceID: 1 },
      60 |         { x: Date.now() - 2000, y: 2, traceID: 2 },

(for some reason they are not included in the CI output).

anikdhabal · 2023-08-29T21:38:05Z

when I run yarn test locally I am getting various errors related to your changes, like this:

 FAIL  src/components/SearchTracePage/SearchResults/ScatterPlot.test.js
  ● Console

    console.error
      Warning: Failed prop type: Invalid prop `data[0].traceID` of type `number` supplied to `ScatterPlot`, expected `string`.
          at data (/Users/ysh/dev/jaegertracing/jaeger-ui/packages/jaeger-ui/src/components/SearchTracePage/SearchResults/ScatterPlot.jsx:33:11)

      55 | it('<ScatterPlot /> should render base case correctly', () => {
      56 |   const wrapper = shallow(
    > 57 |     <ScatterPlot
         |     ^
      58 |       data={[
      59 |         { x: Date.now() - 3000, y: 1, traceID: 1 },
      60 |         { x: Date.now() - 2000, y: 2, traceID: 2 },

(for some reason they are not included in the CI output).

Okay, let me go through it and try to fix it.

anshgoyalevil · 2023-08-29T22:11:09Z

for some reason they are not included in the CI output

I also noticed the same. There are a lot of warnings because of the Deprecated Enzyme API, and due to that, the useful errors cannot be seen in CI output.

A quick fix here can be to use the --silent flag in test-ci script.
This would help suppressing the warnings and console logs, after which the errors can hopefully be seen in CI output.

Though ignoring the warnings by suppressing them is also wrong. In future, we may fix all of the warnings (hopefully), and fail the CI on warning too.

anikdhabal · 2023-08-29T22:20:42Z

for some reason they are not included in the CI output

I also noticed the same. There are a lot of warnings because of the Deprecated Enzyme API, and due to that, the useful errors cannot be seen in CI output.

A quick fix here can be to use the --silent flag in test-ci script. This would help suppressing the warnings and console logs, after which the errors can hopefully be seen in CI output.

Though ignoring the warnings by suppressing them is also wrong. In future, we may fix all of the warnings (hopefully), and fail the CI on warning too.

Thanks for your review. I will definitely try it.

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro · 2023-09-04T17:03:05Z

the two CI builds now very clearly show the breaks, both from linter and compiler

anikdhabal · 2023-09-04T18:58:29Z

the two CI builds now very clearly show the breaks, both from linter and compiler

It gives this error every time: 'TypeError: window.ResizeObserver is not a constructor.

anshgoyalevil · 2023-09-04T19:05:25Z

@anikdhabal

global.ResizeObserver = jest.fn().mockImplementation(() => ({
    observe: jest.fn(),
    unobserve: jest.fn(),
    disconnect: jest.fn(),
}))

Add the above code to packages/jaeger-ui/test/jest-per-test-setup.js or jest.global-setup.js.

It should then work.

Ref: https://stackoverflow.com/questions/64558062/how-to-mock-resizeobserver-to-work-in-unit-tests-using-react-testing-library

priyanshu-kun · 2023-09-30T17:12:28Z

@anikdhabal are you still working on this issue?

anikdhabal · 2023-09-30T17:17:31Z

@anikdhabal are you still working on this issue?

Yeah, I was inactive due to my exam, but I have fixed that issue now.

MarkPollmann · 2024-04-02T08:35:59Z

Can we help to get this shipped?

yurishkuro · 2024-04-02T14:51:02Z

help is welcome, this needs a rebase

MarkPollmann · 2024-04-04T10:33:21Z

Okay, I'm on it

replace react-vis with recharts

69941aa

Signed-off-by: Anik Dhabal Babu <[email protected]>

anikdhabal requested review from everett980 and rubenvp8510 as code owners August 22, 2023 17:44

anikdhabal changed the title ~~Replace react-vis with recharts~~ Replace react-vis with Recharts Aug 22, 2023

yurishkuro reviewed Aug 22, 2023

View reviewed changes

packages/jaeger-ui/src/components/Monitor/ServicesView/index.test.js Show resolved Hide resolved

anshgoyalevil reviewed Aug 22, 2023

View reviewed changes

package.json Outdated Show resolved Hide resolved

anshgoyalevil reviewed Aug 22, 2023

View reviewed changes

packages/jaeger-ui/src/components/Monitor/ServicesView/index.tsx Show resolved Hide resolved

anshgoyalevil reviewed Aug 22, 2023

View reviewed changes

packages/jaeger-ui/src/components/Monitor/ServicesView/operationDetailsTable/opsGraph.tsx Outdated Show resolved Hide resolved

anshgoyalevil reviewed Aug 22, 2023

View reviewed changes

packages/jaeger-ui/src/components/Monitor/ServicesView/serviceGraph.tsx Outdated Show resolved Hide resolved

fix

ea73528

Signed-off-by: Anik Dhabal Babu <[email protected]>

anikdhabal force-pushed the issue#1597 branch from 68b24be to ea73528 Compare August 27, 2023 16:39

Merge branch 'main' into issue#1597

8f7d31d

yurishkuro reviewed Aug 27, 2023

View reviewed changes

anikdhabal and others added 3 commits August 29, 2023 00:20

Merge branch 'main' into issue#1597

cda571d

Signed-off-by: Anik Dhabal Babu <[email protected]>

Update package.json

bb8f409

Signed-off-by: Anik Dhabal Babu <[email protected]>

sizerange added

54031b7

Signed-off-by: Anik Dhabal Babu <[email protected]>

anikdhabal force-pushed the issue#1597 branch from b276ae2 to 54031b7 Compare August 28, 2023 23:02

Merge branch 'main' into issue#1597

3b7d02f

Signed-off-by: Yuri Shkuro <[email protected]>

MarkPollmann mentioned this pull request Apr 5, 2024

Replace react-vis with Recharts #2242

Draft

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace react-vis with Recharts #1722

Replace react-vis with Recharts #1722

anikdhabal commented Aug 22, 2023

yurishkuro left a comment

anshgoyalevil commented Aug 22, 2023

anshgoyalevil Aug 22, 2023 •

edited

Loading

anikdhabal Aug 23, 2023

anshgoyalevil Aug 22, 2023

anikdhabal Aug 23, 2023

anshgoyalevil Aug 22, 2023

anikdhabal Aug 23, 2023

anshgoyalevil Aug 22, 2023

anikdhabal Aug 23, 2023

yurishkuro Aug 27, 2023

anikdhabal Aug 27, 2023

yurishkuro Aug 27, 2023

anikdhabal Aug 28, 2023

yurishkuro commented Aug 27, 2023

anikdhabal commented Aug 27, 2023

yurishkuro commented Aug 27, 2023

anikdhabal commented Aug 29, 2023

yurishkuro commented Aug 29, 2023

anikdhabal commented Aug 29, 2023

anshgoyalevil commented Aug 29, 2023 •

edited

Loading

anikdhabal commented Aug 29, 2023

yurishkuro commented Sep 4, 2023

anikdhabal commented Sep 4, 2023

anshgoyalevil commented Sep 4, 2023

priyanshu-kun commented Sep 30, 2023

anikdhabal commented Sep 30, 2023

MarkPollmann commented Apr 2, 2024

yurishkuro commented Apr 2, 2024

MarkPollmann commented Apr 4, 2024

		yAxisTickFormat?: (v: number) => number;
		yAxisTickFormat?: (v: number) => string;

Replace react-vis with Recharts #1722

Are you sure you want to change the base?

Replace react-vis with Recharts #1722

Conversation

anikdhabal commented Aug 22, 2023

Which problem is this PR solving?

Checklist

yurishkuro left a comment

Choose a reason for hiding this comment

anshgoyalevil commented Aug 22, 2023

anshgoyalevil Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Aug 27, 2023

anikdhabal commented Aug 27, 2023

yurishkuro commented Aug 27, 2023

anikdhabal commented Aug 29, 2023

yurishkuro commented Aug 29, 2023

anikdhabal commented Aug 29, 2023

anshgoyalevil commented Aug 29, 2023 • edited Loading

anikdhabal commented Aug 29, 2023

yurishkuro commented Sep 4, 2023

anikdhabal commented Sep 4, 2023

anshgoyalevil commented Sep 4, 2023

priyanshu-kun commented Sep 30, 2023

anikdhabal commented Sep 30, 2023

MarkPollmann commented Apr 2, 2024

yurishkuro commented Apr 2, 2024

MarkPollmann commented Apr 4, 2024

anshgoyalevil Aug 22, 2023 •

edited

Loading

anshgoyalevil commented Aug 29, 2023 •

edited

Loading