Replace react-vis with recharts #2679
Conversation
Signed-off-by: Hariom Gupta <[email protected]>
Signed-off-by: Hariom Gupta <[email protected]>
Signed-off-by: Hariom Gupta <[email protected]>
Signed-off-by: Hariom Gupta <[email protected]>
Signed-off-by: Hariom Gupta <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @hari45678, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request addresses issue #1597 by replacing the react-vis library with recharts throughout the codebase. This involves updating the package-lock.json and package.json files to remove react-vis and add recharts and its dependencies. The code changes primarily focus on replacing react-vis components with their recharts equivalents in opsGraph.tsx and serviceGraph.tsx, and updating the styling in serviceGraph.css to be compatible with recharts.
Highlights
- Dependency Replacement: The core change is the removal of
react-visand the addition ofrechartsas the primary charting library. This includes installingrechartsand its dependencies, and removingreact-vis. - Component Updates: The
XYPlot,AreaSeries,LineSeriesand other components fromreact-visare replaced withAreaChart,Area,ResponsiveContainer,XAxis,YAxis,Tooltip,Legendand other components fromrechartsinopsGraph.tsxandserviceGraph.tsx. - Styling Adjustments: The CSS file
serviceGraph.cssis updated to include styles specific torecharts, such as.graph-axis,.graph-tooltip,.graph-x-axis, and.graph-y-axis. - ScatterPlot: The
ScatterPlot.jsxfile is updated to userechartscomponents instead ofreact-viscomponents.
Changelog
Click here to see the changelog
- package-lock.json
- Removed
react-visand addedrechartsand its dependencies. - Updated versions of various packages.
- Removed
- packages/jaeger-ui/package.json
- Removed
react-visand addedrechartsas a dependency.
- Removed
- packages/jaeger-ui/src/components/Monitor/ServicesView/operationDetailsTable/opsGraph.tsx
- Replaced
react-viscomponents withrechartscomponents for operation graph rendering.
- Replaced
- packages/jaeger-ui/src/components/Monitor/ServicesView/serviceGraph.css
- Added new CSS classes for styling
rechartscomponents.
- Added new CSS classes for styling
- packages/jaeger-ui/src/components/Monitor/ServicesView/serviceGraph.tsx
- Replaced
react-viscomponents withrechartscomponents for service graph rendering. - Added logic to calculate y-axis domain and ticks.
- Added logic to format y-axis ticks.
- Replaced
- packages/jaeger-ui/src/components/SearchTracePage/SearchResults/ScatterPlot.css
- Removed react-vis specific css classes
- packages/jaeger-ui/src/components/SearchTracePage/SearchResults/ScatterPlot.jsx
- Replaced
react-viscomponents withrechartscomponents for scatter plot rendering. - Added logic to generate unique ticks for the x-axis.
- Replaced
- packages/jaeger-ui/src/components/SearchTracePage/SearchResults/ScatterPlot.test.js
- Updated tests to use react-testing-library instead of enzyme.
- Updated tests to reflect changes in ScatterPlot.jsx
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Did you know?
Recharts is known for its composable architecture, allowing developers to customize charts by combining different components.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request replaces react-vis with recharts throughout the codebase. The changes seem well-structured and address the issue of migrating to a more actively maintained charting library. The use of recharts appears to be implemented correctly, and the removal of react-vis dependencies is thorough. However, there are a few areas where additional context or minor adjustments could improve the code.
Merge Readiness
The pull request appears to be in good shape for merging. The core functionality of replacing react-vis with recharts seems to be implemented correctly. Before merging, it would be beneficial to ensure that all visual aspects of the charts are consistent with the previous implementation and that the new charts are fully accessible. I am unable to approve this pull request, and recommend that others review and approve this code before merging.
|
Tests would fail and opsGraph is not working fine currently. Have created the PR for bundle-check |
There was a problem hiding this comment.
PR Overview
This PR replaces the deprecated react-vis charts with recharts across the Jaeger UI codebase to resolve issue #1597. Key changes include:
- Migrating chart components (XYPlot, LineSeries, AreaSeries, etc.) to recharts equivalents.
- Refactoring utility methods for data transformation and axis tick generation.
- Updating tests to use React Testing Library and providing necessary mocks for recharts.
Reviewed Changes
| File | Description |
|---|---|
| packages/jaeger-ui/src/components/Monitor/ServicesView/serviceGraph.tsx | Replaces react-vis components with recharts components and refactors data handling and chart rendering logic. |
| packages/jaeger-ui/src/components/SearchTracePage/SearchResults/ScatterPlot.jsx | Migrates from react-vis scatter plots to recharts ScatterChart with custom tooltip and dot rendering. |
| packages/jaeger-ui/src/components/SearchTracePage/SearchResults/ScatterPlot.test.js | Updates tests to use React Testing Library and mocks for ResizeObserver and SVG measurement methods. |
| packages/jaeger-ui/src/components/Monitor/ServicesView/operationDetailsTable/opsGraph.tsx | Switches from react-vis charts to recharts AreaChart and updates placeholder text and prop names. |
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Signed-off-by: Hariom Gupta <[email protected]>
Signed-off-by: Hariom Gupta <[email protected]>
Signed-off-by: Hariom Gupta <[email protected]>
Signed-off-by: Hariom Gupta <[email protected]>
Signed-off-by: Hariom Gupta <[email protected]>
Signed-off-by: Hariom Gupta <[email protected]>
Signed-off-by: Hariom Gupta <[email protected]>
Signed-off-by: Hariom Gupta <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2679 +/- ##
==========================================
+ Coverage 96.59% 96.62% +0.03%
==========================================
Files 256 256
Lines 7727 7803 +76
Branches 1950 1961 +11
==========================================
+ Hits 7464 7540 +76
Misses 263 263 ☔ View full report in Codecov by Sentry. |
|
Comparisons of previous vs new implementation:
Previous: Now:
Previous: Now:
Previous: New:
Previous: Now:
Previous: Now:
Previous: Now: |
|
Please let me know if the visual changes look good, and also if this PR needs to be broken into 3 or 2 separate PRs |
| } | ||
|
|
||
| if (dataPoints.length === 0) { | ||
| return OperationsGraph.generatePlaceholder('No Data'); | ||
| } | ||
|
|
||
| const dynProps: { | ||
| yDomain?: yDomain; | ||
| domain?: yDomain; |
There was a problem hiding this comment.
recharts uses domain instead of yDomain as a prop for defining the domain, so we are doing it this way
| const yValues = dataPoints.map(point => point.y).filter((y): y is number => y != null); | ||
| if (yValues.length > 0) { | ||
| const minY = Math.min(...yValues); | ||
| const maxY = Math.max(...yValues); | ||
| if (minY === maxY) { | ||
| dynProps.domain = [minY * 0.8, minY * 2]; | ||
| } else { | ||
| dynProps.domain = [minY * 0.99, maxY]; | ||
| } | ||
| } |
There was a problem hiding this comment.
react-vis use to change manage the domain internally, so for small data points, like 22, 23, 24, it automatically boils down the domain to 22-24.
recharts need an explicit domain dynamic to the data it is rendering. It is kind of zooming into the graph, so those small operation graphs can render small detailing
There was a problem hiding this comment.
This file took a lot of tests and iterations to reach the coverage of 90%+, and now I realize why there were snapshot tests previously.
Charting libs are very hard to test as they inject svgs and other responsive elements into DOM which gets a lot easier with snapshots, while getting a good line coverage
| calculateYAxisTicks(domain: number[]): number[] { | ||
| const [min, max] = domain; | ||
| const step = (max - min) / 4; | ||
| return Array.from({ length: 5 }, (_, i) => min + step * i); |
There was a problem hiding this comment.
Since we did a domain zooming over here, the ticks need to be manually calculated
| calculateNumericTicks(xDomain: number[]): number[] { | ||
| const [start, end] = xDomain; | ||
| const count = 7; | ||
| const step = (end - start) / (count - 1); | ||
|
|
||
| return Array.from({ length: count }, (_, i) => start + step * i); | ||
| } |
There was a problem hiding this comment.
This is the actual zooming logic we did over here. So for example, if a chart have very small values, and we need to show equidistant ticks on y-axis, this function would generate ticks dynamic to the data points
| const generateUniqueTicks = (min, max, count) => { | ||
| const step = (max - min) / (count - 1); | ||
| const ticks = []; | ||
| const seenLabels = new Set(); | ||
|
|
||
| for (let i = 0; i < count; i++) { | ||
| const value = min + step * i; | ||
| ticks.push(value); | ||
| } | ||
|
|
||
| return ticks.filter(tick => { | ||
| const label = dayjs(tick / ONE_MILLISECOND).format('hh:mm:ss a'); | ||
| if (seenLabels.has(label)) { | ||
| return false; | ||
| } | ||
| seenLabels.add(label); | ||
| return true; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Again, this is something we have to do to generate custom equidistant ticks on x-axis
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test