Replace react-vis with recharts #2558

navaneeth-dev · 2025-01-06T17:32:46Z

Which problem is this PR solving?

Resolves Replace react-vis library for drawing charts #1597

Description of the changes

Still work in progress, copying all the changes from previous PR #2242 and implementing the needed tests.

How was this change tested?

npm run start
npm run build
npm run test

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: npm run lint and npm run test

navaneeth-dev · 2025-01-07T12:25:56Z

@yurishkuro My first step is replacing the search page and get the tests working, then move on to other pages.

Changes made that are different from the original PR:

Fixes non-uniform time: Max 4 ticks, interval evenly calculated from start and end in generateTicks()
Made Y Axis label "Trace duration"

Which should we use? Should the cells clip at the edge like the second one?

TODO: Working on making cell size similar to the current behavior.

Thanks in advance.

yurishkuro · 2025-01-07T14:44:25Z

Aside from color scheme the chart looks ok to me. Just make sure all existing behaviors are supported, like hover.

I would add more x-axis ticks. How does grafana do it?

navaneeth-dev · 2025-01-07T15:17:01Z

Yes hover is already working.

Grafana does intervals according to scrape_interval in Prometheus (default 15 seconds).

yurishkuro · 2025-01-07T16:27:41Z

Grafana does intervals according to scrape_interval in Prometheus

but what if I select a larger time range? Doing ticks at scrape intervals would cause labels to overlap.

yurishkuro · 2025-01-07T16:28:02Z

please make sure you work off up to date main branch, you already have merge conflicts

navaneeth-dev · 2025-01-07T16:36:16Z

but what if I select a larger time range? Doing ticks at scrape intervals would cause labels to overlap.

Just checked Grafana, for 2 days they do 2 hours intervals and even when resizing they reduce the tick count.

Do you want Grafana like behavior?

yurishkuro · 2025-01-07T16:46:47Z

Do you want Grafana like behavior?

I don't care about it being specifically Grafana-like, just that the ticks need to adjust depending on the time range of the query, but also the screen size. E.g. in your screenshot you can easily go from 4 to 8 ticks, which will increase readability, but if I start reducing the screen width the 8 ticks might start overlapping, so what is the solution to that?

navaneeth-dev · 2025-01-07T18:14:11Z

Fixed merge conflicts. Do you recommend rebasing every day before starting work on a feature?

We can make it responsive by calculating number of ticks based on containerWidth variable using some math functions with fixed size of date tick size. Meaning, check how many of ticks fits inside containerWidth.

For intervals, it might be a bit more complicated.
For example, Grafana sets interval to 12 hours for 90 day query and returns only those data. Do we have a similar system here? I already hang if I try to get large data like 50 results.

We can calculate the interval like end-start and some if conditions based on the value.

I will try to implement code tomorrow.

navaneeth-dev · 2025-01-08T04:46:19Z

I tried the interval={equidistantPreserveStart} option, but it is NOT equidistant in the middle.

There is an issue open but no timeline: recharts/recharts#3305

I think for now we have to manually calculate it.

yurishkuro · 2025-01-09T23:45:23Z

@navaneeth-dev lmk when ready for review

navaneeth-dev · 2025-01-10T03:35:05Z

Will do, working on migrating tests from emyze to react-testing-library.

Have you thought about adding .editorconfig?

yurishkuro · 2025-01-10T05:27:03Z

working on migrating tests from emyze to react-testing-library.

is there a lot of tests? Perhaps we can merge them in a separate PR before merging this one.

Have you thought about adding .editorconfig?

I don't know what that is, sounds like specific to an IDE that you are using.

navaneeth-dev · 2025-01-10T06:05:00Z

No just started work on ScatterPlot.test.js, was busy yesterday. So do you want me to finish ScatterPlot component fully with tests and let know know for this PR?

"Editorconfig helps maintain consistent coding styles for multiple developers working on the same project across various editors and IDEs":

So formatter (prettier) usually comes after saving, but EditorConfig sets up the editor's: space, indent size correctly etc...
I highly recommend it and many CNCF projects use it. Currently, I have it git excluded with Neovim.
https://editorconfig.org/

yurishkuro · 2025-01-11T06:04:23Z

my point was that changing tests to react-testing-library is a task on its own, it's good to do it before making changes to the code, such that we can then actually just the later code changes based on whether the tests are passing.

navaneeth-dev · 2025-01-11T07:31:14Z

Understood, will migrate the remaining files and let you know:

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

navaneeth-dev · 2025-01-24T18:40:34Z

What is the purpose of a Line/LineSeries here? In my demo instance, I can't see any lines.
https://github.com/MarkPollmann/jaeger-ui/blob/e8c58019494cc755bc0f6efad86af5eb00f9646f/packages/jaeger-ui/src/components/Monitor/ServicesView/serviceGraph.tsx#L93

navaneeth-dev · 2025-01-25T06:59:19Z

@yurishkuro

Service Graph latency, error, request rate graphs finished. Is the tooltip fine?

output.mp4

Bottom graphs seem to have some bug, will be fixing that.

navaneeth-dev · 2025-01-25T12:13:42Z

Fixed opsGraph, this PR is now ready.

I have two questions:

Error rate data point's y value IS 0, so I set minimum value to 0.1 for every opsGraph so it displays a red line.
Do we need sharp steps like below OR like my previous video which is linear? When the service is down, there is no data so there is a gap.

EDIT: I fixed the order of time to behave like current main one.
EDIT2: I still don't know the purpose of LineSeries.

yurishkuro

please resolve conflicts and update branch up to date

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

yurishkuro · 2025-01-26T01:25:47Z

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

    } = {};

    if (yDomain) {
-      dynProps.yDomain = yDomain;
+      dynProps.domain = yDomain;


please avoid unnecessary changes, I want to focus my review on the main changes, not be distracted by these ^^^

This is a necessary change, <YAxis hide={true} {...dynProps} /> YAxis takes domain prop not yDomain like react-vis.

How would you want me to proceed?

do you need this const and if at all? Could it not be just inlined where needed?

packages/jaeger-ui/src/components/SearchTracePage/SearchResults/ScatterPlot.jsx

Signed-off-by: Navaneeth Rao <[email protected]>

- Equal interval - Proper Label position Signed-off-by: Navaneeth Rao <[email protected]>

Signed-off-by: Navaneeth Rao <[email protected]>

navaneeth-dev

Fixed all merge conflicts and updated to main.

Signed-off-by: Navaneeth Rao <[email protected]>

yurishkuro · 2025-01-26T16:12:19Z

packages/jaeger-ui/package.json

@@ -82,6 +82,7 @@
    "react-vis-force": "^0.3.1",


is react-vis still needed after this change?

No, I will remove it

yurishkuro

I ran monitor example. The Search screen did not show scatterplot at all

The Monitor page showed charts, but their time axis doesn't make sense, when I switched to 24hrs view:

navaneeth-dev · 2025-01-26T17:19:29Z

react-vis-force is different from react-vis. Once we migrate the tests we can remove react-vis.

rg react-vis                                     
package.json
49:    "prettier": "prettier --write '{.,scripts}/*.{js,jsx,json,md,ts,tsx,mts}' 'packages/*/{src,demo/src}/**/!(layout.worker.bundled|react-vis).{css,js,jsx,json,md,ts,tsx}' 'packages/*/*.{css,js,jsx,json,md,ts,tsx,mts}'",
50:    "prettier-lint": "prettier --list-different '{.,scripts}/*.{js,jsx,json,md,ts,tsx,mts}' 'packages/*/{src,demo/src}/**/!(layout.worker.bundled|react-vis).{css,js,jsx,json,md,ts,tsx}' 'packages/*/*.{css,js,jsx,json,md,ts,tsx,mts}' || (echo '*** PLEASE RUN npm run prettier AND RESUBMIT ***' && false)",

packages/jaeger-ui/package.json
81:    "react-vis": "1.11.12",
82:    "react-vis-force": "^0.3.1",

packages/jaeger-ui/typings/custom.d.ts
43:declare module 'react-vis-force';

package-lock.json
16602:    "node_modules/react-vis": {
16604:      "resolved": "https://registry.npmjs.org/react-vis/-/react-vis-1.11.12.tgz",
16635:    "node_modules/react-vis-force": {
16637:      "resolved": "https://registry.npmjs.org/react-vis-force/-/react-vis-force-0.3.1.tgz",
16651:    "node_modules/react-vis/node_modules/d3-color": {
16657:    "node_modules/react-vis/node_modules/d3-interpolate": {
16666:    "node_modules/react-vis/node_modules/deep-equal": {
20264:        "react-vis": "1.11.12",
20265:        "react-vis-force": "^0.3.1",

packages/jaeger-ui/src/components/DependencyGraph/ForceGraphArrowLink.tsx
22:import { ForceGraphLink } from 'react-vis-force';

packages/jaeger-ui/src/components/DependencyGraph/DependencyForceGraph.test.js
17:import { InteractiveForceGraph, ForceGraphNode } from 'react-vis-force';

packages/jaeger-ui/src/components/DependencyGraph/DependencyForceGraph.jsx
16:import { InteractiveForceGraph, ForceGraphNode } from 'react-vis-force';

packages/jaeger-ui/src/components/SearchTracePage/SearchResults/ScatterPlot.test.js
17:import { XAxis, XYPlot, YAxis, MarkSeries, Hint } from 'react-vis';

navaneeth-dev · 2025-01-26T17:23:17Z

I think there is a bug with the size of ScatterPlot circles will test and let you know. I ran monitor docker compose example too but it shows for me 🤔 .

As for Monitor I used default recharts xaxis so interval isn't uniform. I will try to make it like Grafana and let you know.

yurishkuro · 2025-01-26T17:25:47Z

react-vis-force is different from react-vis.

I know, I was referring to react-vis only.

navaneeth-dev force-pushed the replace-react-vis branch from ae0cbf8 to 4277922 Compare January 7, 2025 12:25

navaneeth-dev force-pushed the replace-react-vis branch 2 times, most recently from 84a5b73 to 5a4e614 Compare January 7, 2025 17:54

navaneeth-dev force-pushed the replace-react-vis branch from 98bc560 to 3957421 Compare January 9, 2025 14:23

navaneeth-dev force-pushed the replace-react-vis branch from 3957421 to 186318f Compare January 11, 2025 05:49

navaneeth-dev force-pushed the replace-react-vis branch from 186318f to bd5a6ef Compare January 24, 2025 16:32

yurishkuro reviewed Jan 26, 2025

View reviewed changes

navaneeth-dev added 5 commits January 26, 2025 10:08

Install recharts

bea70ec

Signed-off-by: Navaneeth Rao <[email protected]>

ScatterPlot replace with recharts

28778a6

- Equal interval - Proper Label position Signed-off-by: Navaneeth Rao <[email protected]>

Remove defaultProps to fix deprecation warning

980720f

Signed-off-by: Navaneeth Rao <[email protected]>

Update serviceGraph to recharts

6213c70

Signed-off-by: Navaneeth Rao <[email protected]>

Fixed xAxis time order

edfc34f

Signed-off-by: Navaneeth Rao <[email protected]>

Updated opsGraph to recharts

7e28fe8

Signed-off-by: Navaneeth Rao <[email protected]>

navaneeth-dev force-pushed the replace-react-vis branch from cc45554 to 7e28fe8 Compare January 26, 2025 04:41

navaneeth-dev added 2 commits January 26, 2025 10:20

Revert formatter change for ServicesView/index.tsx

de87ba7

Signed-off-by: Navaneeth Rao <[email protected]>

Remove unnecessary useRef code in ScatterPlot

8c3ae1e

Signed-off-by: Navaneeth Rao <[email protected]>

navaneeth-dev commented Jan 26, 2025

View reviewed changes

navaneeth-dev marked this pull request as ready for review January 26, 2025 05:08

navaneeth-dev requested a review from a team as a code owner January 26, 2025 05:08

navaneeth-dev requested review from pavolloffay and removed request for a team January 26, 2025 05:08

Added pointer cursor to ScatterPlot circles

d8e85eb

Signed-off-by: Navaneeth Rao <[email protected]>

yurishkuro reviewed Jan 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace react-vis with recharts #2558

Replace react-vis with recharts #2558

navaneeth-dev commented Jan 6, 2025 •

edited

Loading

navaneeth-dev commented Jan 7, 2025 •

edited

Loading

yurishkuro commented Jan 7, 2025

navaneeth-dev commented Jan 7, 2025 •

edited

Loading

yurishkuro commented Jan 7, 2025

yurishkuro commented Jan 7, 2025

navaneeth-dev commented Jan 7, 2025

yurishkuro commented Jan 7, 2025

navaneeth-dev commented Jan 7, 2025

navaneeth-dev commented Jan 8, 2025

yurishkuro commented Jan 9, 2025

navaneeth-dev commented Jan 10, 2025

yurishkuro commented Jan 10, 2025

navaneeth-dev commented Jan 10, 2025

yurishkuro commented Jan 11, 2025

navaneeth-dev commented Jan 11, 2025

navaneeth-dev commented Jan 24, 2025

navaneeth-dev commented Jan 25, 2025

navaneeth-dev commented Jan 25, 2025 •

edited

Loading

yurishkuro left a comment

yurishkuro Jan 26, 2025

navaneeth-dev Jan 26, 2025

yurishkuro Jan 26, 2025

navaneeth-dev left a comment

yurishkuro Jan 26, 2025

navaneeth-dev Jan 26, 2025

yurishkuro left a comment

navaneeth-dev commented Jan 26, 2025

navaneeth-dev commented Jan 26, 2025

yurishkuro commented Jan 26, 2025

Replace react-vis with recharts #2558

Are you sure you want to change the base?

Replace react-vis with recharts #2558

Conversation

navaneeth-dev commented Jan 6, 2025 • edited Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

navaneeth-dev commented Jan 7, 2025 • edited Loading

yurishkuro commented Jan 7, 2025

navaneeth-dev commented Jan 7, 2025 • edited Loading

yurishkuro commented Jan 7, 2025

yurishkuro commented Jan 7, 2025

navaneeth-dev commented Jan 7, 2025

yurishkuro commented Jan 7, 2025

navaneeth-dev commented Jan 7, 2025

navaneeth-dev commented Jan 8, 2025

yurishkuro commented Jan 9, 2025

navaneeth-dev commented Jan 10, 2025

yurishkuro commented Jan 10, 2025

navaneeth-dev commented Jan 10, 2025

yurishkuro commented Jan 11, 2025

navaneeth-dev commented Jan 11, 2025

navaneeth-dev commented Jan 24, 2025

navaneeth-dev commented Jan 25, 2025

navaneeth-dev commented Jan 25, 2025 • edited Loading

yurishkuro left a comment

Choose a reason for hiding this comment

yurishkuro Jan 26, 2025

Choose a reason for hiding this comment

navaneeth-dev Jan 26, 2025

Choose a reason for hiding this comment

yurishkuro Jan 26, 2025

Choose a reason for hiding this comment

navaneeth-dev left a comment

Choose a reason for hiding this comment

yurishkuro Jan 26, 2025

Choose a reason for hiding this comment

navaneeth-dev Jan 26, 2025

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

navaneeth-dev commented Jan 26, 2025

navaneeth-dev commented Jan 26, 2025

yurishkuro commented Jan 26, 2025

navaneeth-dev commented Jan 6, 2025 •

edited

Loading

navaneeth-dev commented Jan 7, 2025 •

edited

Loading

navaneeth-dev commented Jan 7, 2025 •

edited

Loading

navaneeth-dev commented Jan 25, 2025 •

edited

Loading