upcoming: [DI-21200] - Migration from ChartJS to Recharts in CloudPulse #11204

nikhagra-akamai · 2024-11-04T08:14:27Z

Description 📝

Migrated ChartJS to Recharts in CloudPulse.

Note: This PR is raised so that code changes can be reviewed & parallelly we are working on automation test cases which will eventually be added in this PR.

Changes 🔄

List any change relevant to the reviewer.

Replaced LineGraph with AreaChart in CloudPulseLineGraph component.
Updated x-axis ticks & chart variant in CloudPulseWidget
Modified structure the of data passed to AreaChart component in CloudPulseWidgetUtils
Removed CloudPulseColorPalette
Modified legendColor property in metrics display to accept any color
Added a connectNulls property in AreaChart component
Added DataSet type in cloudpulse types

Target release date 🗓️

18th November

Preview 📷

Include a screenshot or screen recording of the change.

🔒 Use the Mask Sensitive Data setting for security.

💡 Use <video src="" /> tag when including recordings in table.

No Data Display

X-Axis with timestamp ticks for less than 24hrs

X-Axis with date & timestamp ticks for more than 24hrs

Legends rows
https://github.com/user-attachments/assets/bc8886ba-db7a-44ba-aa09-fea7d715bd22

Error State

How to test 🧪

Switch to mock user
Select mandatory filters to generate the graphs
Change time duration filter to see difference in x-axis tick
Modify data in serverHandlers.ts for monitor/services/:serviceType/metrics endpoint to playaround with graph & legends.
You can see a scroll bar in legend rows table if more than 2 legends are present

As an Author I have considered 🤔

Check all that apply

abailly-akamai · 2024-11-04T14:33:04Z

@nikhagra-akamai please address the e2e failures before we get to the review - thx!

hkhalil-akamai · 2024-11-04T22:39:06Z

packages/manager/src/components/LineGraph/MetricsDisplay.tsx

@@ -47,7 +38,7 @@ export interface MetricsDisplayRow {
  data: Metrics;
  format: (n: number) => string;
  handleLegendClick?: () => void;
-  legendColor: LegendColor;
+  legendColor: string;


This looks worse from a type safety perspective. Is this change necessary?

yes it is already been discussed with @jaalah-akamai that instead of restricting to the 7 predefined colors, it should accept any color, so that legend rows greater than 7 won't collide with same color. Also, I don't think color has anything related to typesafety.

hkhalil-akamai · 2024-11-04T22:54:23Z

packages/api-v4/src/cloudpulse/types.ts

+export interface DataSet {
+  [label: string]: number;
+  timestamp: number;
+}


Interfaces in the API SDK should correspond to responses from a particular endpoint. This definition may be more appropriately located in CloudPulseWidgetUtils.ts.

updated, thanks

hkhalil-akamai · 2024-11-04T22:55:47Z

packages/manager/src/components/AreaChart/AreaChart.tsx

+  /**
+   * connect nulls value between two data points
+   */
+  connectNulls? :boolean;


Suggested change

connectNulls? :boolean;

connectNulls?: boolean;

updated, it was there in local changes but forgot to push. Thanks

hkhalil-akamai · 2024-11-04T23:07:25Z

packages/manager/src/features/CloudPulse/Widget/components/CloudPulseLineGraph.tsx

-          legendRows={legendRows}
+          connectNulls
+          fillOpacity={0.5}
+          legendHeight={theme.spacing(18.75)}


IMO, the overflow in the legend is worse from a usability perspective -- I would prefer to be able to see all rows without a scroll bar.

This is also already been discussed. I understand your concern, but think for dynamically growing legend rows where number can cross 5. Bigger legend row table won't be look so good for UI and if two side by side graphs have different number of legend row count, then that mismatch in graph & legend height will make it look more worse. As in our case legends rows are dynamic & they can grow to 20.

Understood, thanks for the context.

…eGraph

github-actions · 2024-11-05T07:31:57Z

Coverage Report: ✅
Base Coverage: 87.33%
Current Coverage: 87.34%

nikhagra-akamai · 2024-11-05T09:31:36Z

@nikhagra-akamai please address the e2e failures before we get to the review - thx!

@abailly-akamai This PR is raised so that code changes can be reviewed & parallelly we are working on automation test cases, which will eventually be added in this PR. Please let me know if automation is necessary for the review, I'll make it draft & open once automations are done. - Thx

abailly-akamai · 2024-11-05T15:14:36Z

@nikhagra-akamai I am unsure what you meant, but this just can't be merged with failing tests no matter what cause everyone would inherit these failures

nikhagra-akamai · 2024-11-05T15:25:17Z

@nikhagra-akamai I am unsure what you meant, but this just can't be merged with failing tests no matter what cause everyone would inherit these failures

I only meant that failure tests should not be blocking the PR review. By the time tests are being fixed ( that will be done within a day or 2 ), we can have review for our rest of the PR code.

abailly-akamai · 2024-11-05T15:33:31Z

@nikhagra-akamai fixing the tests may affect the current code and we are trying to minimize the back and forth.

kmuddapo · 2024-11-06T05:35:24Z

@nikhagra-akamai fixing the tests may affect the current code and we are trying to minimize the back and forth.

@nikhagra-akamai is on OOO for this week. May be @abailly-akamai / @jaalah In that case could we please move this PR to draft or even close it? That way once team is ready with E2E automations which is currently in progress, we will open up a consolidated PR.

…ration

linode-gh-bot · 2024-11-13T13:38:25Z

Cloud Manager UI test results

🔺 6 failing tests on test run #11 ↗︎

❌ Failing	✅ Passing	↪️ Skipped	🕐 Duration
`6 Failing`	`439 Passing`	`2 Skipped`	`116m 56s`

Details

Failing Tests
	Spec	Test
❌	`dbaas-widgets-verification.spec.ts`	Integration Tests for DBaaS Dashboard » should allow users to select their desired granularity and see the most recent data from the API reflected in the graph
❌	`dbaas-widgets-verification.spec.ts`	Integration Tests for DBaaS Dashboard » should allow users to select the desired aggregation and view the latest data from the API displayed in the graph
❌	`dbaas-widgets-verification.spec.ts`	Integration Tests for DBaaS Dashboard » should zoom in and out of all the widgets
❌	`linode-widget-verification.spec.ts`	Integration Tests for Linode Dashboard » should allow users to select their desired granularity and see the most recent data from the API reflected in the graph
❌	`linode-widget-verification.spec.ts`	Integration Tests for Linode Dashboard » should allow users to select the desired aggregation and view the latest data from the API displayed in the graph
❌	`linode-widget-verification.spec.ts`	Integration Tests for Linode Dashboard » should zoom in and out of all the widgets

Troubleshooting

Use this command to re-run the failing tests:

yarn cy:run -s "cypress/e2e/core/cloudpulse/dbaas-widgets-verification.spec.ts,cypress/e2e/core/cloudpulse/dbaas-widgets-verification.spec.ts,cypress/e2e/core/cloudpulse/dbaas-widgets-verification.spec.ts,cypress/e2e/core/cloudpulse/linode-widget-verification.spec.ts,cypress/e2e/core/cloudpulse/linode-widget-verification.spec.ts,cypress/e2e/core/cloudpulse/linode-widget-verification.spec.ts"

nikhagra-akamai added 12 commits November 4, 2024 13:27

upcoming: [DI-20870] - Migrated chartjs to recharts for ACLP graphs

7be47af

upcoming: [DI-20870] - Removed unused imports

c1d1808

upcoming: [DI-20870] - Removed CloudPulseWidgetColorPalette file

aa9931a

upcoming: [DI-20870] - Optimized code

48fb7bb

upcoming: [DI-21200] - Added connectNulls & color properties

8482608

upcoming: [DI-21200] - Added sort data based on timestamp

801ae23

upcoming: [DI-21200] - Updated x-axis tick format

3fc7ba9

upcoming: [DI-21200] - Updated PR comments

ef29f34

upcoming: [DI-21200] - Hide legends if not data present

1fda168

upcoming: [DI-21200] - Updated legends height

b1b5248

upcoming: [DI-21200] - Updated legend row color type

144818a

upcoming: [DI-21200] - Code refactor

62a5f98

nikhagra-akamai force-pushed the recharts_migration branch from 481e709 to 62a5f98 Compare November 4, 2024 09:39

upcoming: [DI-21200] - Added changeset

a979311

nikhagra-akamai self-assigned this Nov 4, 2024

nikhagra-akamai added the CloudPulse label Nov 4, 2024

nikhagra-akamai marked this pull request as ready for review November 4, 2024 14:22

nikhagra-akamai requested a review from a team as a code owner November 4, 2024 14:22

nikhagra-akamai requested review from hkhalil-akamai and abailly-akamai and removed request for a team November 4, 2024 14:22

hkhalil-akamai reviewed Nov 4, 2024

View reviewed changes

nikhagra-akamai added 2 commits November 5, 2024 12:18

upcoming: [DI-21200] - Moved DataSet type from types to CloudPulseLin…

75adf4d

…eGraph

upcoming: [DI-21200] - Synced with upstream develop

f36786d

jaalah-akamai added the Requires Changes label Nov 7, 2024

abailly-akamai marked this pull request as draft November 7, 2024 14:20

nikhagra-akamai added 5 commits November 12, 2024 15:15

Merge branch 'develop' of github.com:linode/manager into recharts_mig…

6026adc

…ration

upcoming: [DI-21200] - updated height of chart & legend rows

b8113b3

upcoming: [DI-21200] - Updated no data display positioning

effca04

upcoming: [DI-21200] - Merge conflicts resolved

abc1fe3

upcoming: [DI-21841] - Removed connect nulls

de10d79

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upcoming: [DI-21200] - Migration from ChartJS to Recharts in CloudPulse #11204

upcoming: [DI-21200] - Migration from ChartJS to Recharts in CloudPulse #11204

nikhagra-akamai commented Nov 4, 2024 •

edited

Loading

abailly-akamai commented Nov 4, 2024

hkhalil-akamai Nov 4, 2024

nikhagra-akamai Nov 5, 2024 •

edited

Loading

hkhalil-akamai Nov 4, 2024

nikhagra-akamai Nov 5, 2024

hkhalil-akamai Nov 4, 2024

nikhagra-akamai Nov 5, 2024 •

edited

Loading

hkhalil-akamai Nov 4, 2024

nikhagra-akamai Nov 5, 2024

hkhalil-akamai Nov 5, 2024

github-actions bot commented Nov 5, 2024

nikhagra-akamai commented Nov 5, 2024 •

edited

Loading

abailly-akamai commented Nov 5, 2024

nikhagra-akamai commented Nov 5, 2024

abailly-akamai commented Nov 5, 2024

kmuddapo commented Nov 6, 2024

linode-gh-bot commented Nov 13, 2024

upcoming: [DI-21200] - Migration from ChartJS to Recharts in CloudPulse #11204

Are you sure you want to change the base?

upcoming: [DI-21200] - Migration from ChartJS to Recharts in CloudPulse #11204

Conversation

nikhagra-akamai commented Nov 4, 2024 • edited Loading

Description 📝

Changes 🔄

Target release date 🗓️

Preview 📷

How to test 🧪

As an Author I have considered 🤔

abailly-akamai commented Nov 4, 2024

hkhalil-akamai Nov 4, 2024

Choose a reason for hiding this comment

nikhagra-akamai Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

hkhalil-akamai Nov 4, 2024

Choose a reason for hiding this comment

nikhagra-akamai Nov 5, 2024

Choose a reason for hiding this comment

hkhalil-akamai Nov 4, 2024

Choose a reason for hiding this comment

nikhagra-akamai Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

hkhalil-akamai Nov 4, 2024

Choose a reason for hiding this comment

nikhagra-akamai Nov 5, 2024

Choose a reason for hiding this comment

hkhalil-akamai Nov 5, 2024

Choose a reason for hiding this comment

github-actions bot commented Nov 5, 2024

nikhagra-akamai commented Nov 5, 2024 • edited Loading

abailly-akamai commented Nov 5, 2024

nikhagra-akamai commented Nov 5, 2024

abailly-akamai commented Nov 5, 2024

kmuddapo commented Nov 6, 2024

linode-gh-bot commented Nov 13, 2024

Cloud Manager UI test results

🔺 6 failing tests on test run #11 ↗︎

Details

Troubleshooting

nikhagra-akamai commented Nov 4, 2024 •

edited

Loading

nikhagra-akamai Nov 5, 2024 •

edited

Loading

nikhagra-akamai Nov 5, 2024 •

edited

Loading

nikhagra-akamai commented Nov 5, 2024 •

edited

Loading