Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

upcoming: [DI-19818] - Added line graph to CloudPulse widgets for different metrics #10710

Merged
merged 12 commits into from
Aug 2, 2024

Conversation

nikhagra-akamai
Copy link
Contributor

@nikhagra-akamai nikhagra-akamai commented Jul 25, 2024

Description 📝

Line graph component is added in cloud pulse widget to showcase data for different metrics.

Changes 🔄

List any change relevant to the reviewer.

  1. CloudPulseLineGraph is added in the cloud pulse widget for metrics
  2. LineGraph component is modified for full width legends as it was restricting to 85% width
  3. Widget color palette is added for predefined colors in graph
  4. Necessary type definitions are added to support metrics data
  5. Added the metrics api call outside api-v4

Target release date 🗓️

2nd August 2024

Preview 📷

Include a screenshot or screen recording of the change

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

Before After
Screenshot 2024-07-25 at 3 17 07 PM Screenshot 2024-07-25 at 3 05 37 PM
Screenshot 2024-07-25 at 3 05 44 PM Screenshot 2024-07-25 at 3 05 56 PM
Screenshot 2024-07-25 at 7 30 44 PM Screenshot 2024-07-25 at 7 31 02 PM

How to test 🧪

  1. Enable mock services as some endpoints are not ready to use.
  2. Graph will be populated with dummy data so you can see the visualization & tool tips.

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@nikhagra-akamai nikhagra-akamai requested a review from a team as a code owner July 25, 2024 09:49
@nikhagra-akamai nikhagra-akamai requested review from carrillo-erik and cpathipa and removed request for a team July 25, 2024 09:49
Copy link

github-actions bot commented Jul 25, 2024

Coverage Report:
Base Coverage: 82.43%
Current Coverage: 82.45%

Copy link
Contributor

@carrillo-erik carrillo-erik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some initial feedback to be addressed. I'll continue to review but this should give you a starting point. Reach out if you have questions.

@nikhagra-akamai
Copy link
Contributor Author

Left some initial feedback to be addressed. I'll continue to review but this should give you a starting point. Reach out if you have questions.

Sure thanks for comments, I'll addressed them and also put bigger logic into a util file and reduce the component size

Copy link
Contributor

@venkymano-akamai venkymano-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jaalah-akamai
Copy link
Contributor

There seems to be a lot of jankiness when you change values for the charts, I'm not sure if that's because we're dealing with mocks or if it's due to multiple re-renderings. It's just something to keep an eye on as development progresses 👍

packages/manager/src/queries/cloudpulse/metrics.ts Outdated Show resolved Hide resolved
packages/manager/src/queries/cloudpulse/metrics.ts Outdated Show resolved Hide resolved
packages/manager/src/queries/cloudpulse/metrics.ts Outdated Show resolved Hide resolved
packages/manager/src/queries/cloudpulse/metrics.ts Outdated Show resolved Hide resolved
@jaalah-akamai
Copy link
Contributor

Approving pending changes request from Banks

@jaalah-akamai
Copy link
Contributor

Looks like you have a test failure for objectStorage/access-key.e2e.spec.ts try running yarn cy:run -s cypress/e2e/core/objectStorage/access-key.e2e.spec.ts

@jaalah-akamai
Copy link
Contributor

@carrillo-erik this should be ready for another pass 👍

@jaalah-akamai jaalah-akamai added the Add'tl Approval Needed Waiting on another approval! label Jul 31, 2024
@nikhagra-akamai
Copy link
Contributor Author

Looks like you have a test failure for objectStorage/access-key.e2e.spec.ts try running yarn cy:run -s cypress/e2e/core/objectStorage/access-key.e2e.spec.ts

@jaalah-akamai it seems some other test is failing as it doesn't belong to ACLP

@bnussman-akamai
Copy link
Member

I highly recommend that the CloudPulse use recharts instead of chart.js

@nikhagra-akamai
Copy link
Contributor Author

I highly recommend that the CloudPulse use recharts instead of chart.js

We have already discussed regarding this & for now we can move with chart.js for our beta release.
FYI conversation : https://linode.slack.com/archives/C4V05M8TS/p1720592089825549

@jdamore-linode
Copy link
Contributor

@nikhagra Could you merge in the latest changes from develop? That should resolve the test failures

@nikhagra-akamai
Copy link
Contributor Author

@nikhagra Could you merge in the latest changes from develop? That should resolve the test failures

@jdamore-linode updated from latest dev branch

@bnussman-akamai
Copy link
Member

I'm aware of that Slack discussion, but I don't agree with the outcome. It's just creating more work for us later. @nikhagra

@kmuddapo
Copy link

kmuddapo commented Aug 2, 2024

I highly recommend that the CloudPulse use recharts instead of chart.js

Its in the radar already post discussion on the slack , we will be migrating it post beta asap. Hope this shouldn't be a showstopper for this PR and beta which we have a very tight-timeline of 2 more weeks only for dev integrations with service owners. Thanks!!

@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Aug 2, 2024
@kmuddapo
Copy link

kmuddapo commented Aug 2, 2024

Thanks!! @bnussman-akamai for the approvals. Can this request be merged now please?

@nikhagra-akamai
Copy link
Contributor Author

@jaalah-akamai @jdamore-linode @bnussman-akamai I've marked marked all the conversations as resolved but still it says merging blocked because of requested changes. Could you please check and merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! CloudPulse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants