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

feat: [M3-7487] - Add AGLB Endpoint Health #10008

Merged

Conversation

bnussman-akamai
Copy link
Member

@bnussman-akamai bnussman-akamai commented Dec 19, 2023

Description 📝

  • Adds Endpoint Health information to AGLB

Preview 📷

Before After
Screenshot 2023-12-19 at 3 06 52 PM Screenshot 2023-12-19 at 3 05 46 PM
Screenshot 2023-12-19 at 3 07 24 PM Screenshot 2023-12-19 at 3 05 39 PM
Screenshot 2023-12-19 at 3 07 10 PM Screenshot 2023-12-19 at 3 10 39 PM
Screenshot 2023-12-19 at 3 06 59 PM Screenshot 2023-12-19 at 3 05 54 PM

How to test 🧪

Prerequisites

  • Use the dev environment
  • Login to the dev-test-aglb account

Verification steps

  • Verify that you see health data in all of the places shown in the Preview section

As an Author I have considered 🤔

  • 👀 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

@bnussman-akamai bnussman-akamai added Work in Progress ACLB Relating to the Akamai Cloud Load Balancer labels Dec 19, 2023
@bnussman-akamai bnussman-akamai self-assigned this Dec 19, 2023
@bnussman-akamai bnussman-akamai added Blocked Waiting on other work to be done before this and removed Work in Progress labels Dec 21, 2023
@bnussman-akamai bnussman-akamai removed the Blocked Waiting on other work to be done before this label Jan 10, 2024
@bnussman-akamai bnussman-akamai marked this pull request as ready for review January 10, 2024 20:42
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner January 10, 2024 20:42
@bnussman-akamai bnussman-akamai requested review from carrillo-erik and abailly-akamai and removed request for a team January 10, 2024 20:42
Copy link

github-actions bot commented Jan 11, 2024

Coverage Report:
Base Coverage: 79.83%
Current Coverage: 79.83%

Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Looks great - approving pending minor fixes

also, more of a UX question, is it worth showing the status icon at all in this case?
Screenshot 2024-01-16 at 09 35 09

@@ -0,0 +1,5 @@
---
"@linode/api-v4": Added
---
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm, this isn't an upcoming feature anymore, is it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to keep considering it an upcoming feature until it's in public beta. We're probably a few weeks away from that

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to keep considering it an upcoming feature until it's in public beta.

In that case, should this changeset entry be under Upcoming Features rather than Added?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I wen't with added because api-v4 has no way to to "gate" features. Once the code is released, it's "added" to the package

Copy link
Member Author

Choose a reason for hiding this comment

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

Should it still be Upcoming Features?

Copy link
Contributor

Choose a reason for hiding this comment

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

Once the code is released, it's "added" to the package

Yeah, that's fair. I was thinking we'd been putting other AGLB api-v4 work under Upcoming, so suggested it for consistency, but I might be misremembering.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm not sure. Maybe api-v4 shouldn't have an Upcoming Features? I'll add a cafe item


if (!health) {
return <EndpointHealth down={0} up={0} />;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What could health being undefined mean? Is there anything relevant we'd want to display to the user?

Copy link
Member Author

Choose a reason for hiding this comment

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

It shouldn't really happen because the loading and error cases are checked before this. This was mostly just to satisfy typescript.

@@ -9,6 +9,7 @@ import { TableRow } from 'src/components/TableRow';
import { Typography } from 'src/components/Typography';
import { IPAddress } from 'src/features/Linodes/LinodesLanding/IPAddress';

import { LoadBalancerEndpontHeath } from '../LoadBalancerDetail/LoadBalancerEndpointHealth';
Copy link
Contributor

@abailly-akamai abailly-akamai Jan 16, 2024

Choose a reason for hiding this comment

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

Got two typos on your export LoadBalancerEndpontHeath > LoadBalancerEndpointHealth

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 36ee5b8, good catch!

@bnussman-akamai bnussman-akamai added the Add'tl Approval Needed Waiting on another approval! label Jan 16, 2024
Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Endpoints are visible:
✅ on the Load Balancer Landing page
✅ on the Load Balancer details Summary tab
✅ in the Configurations tab
✅ in the Service Targets table

Edit:

also, more of a UX question, is it worth showing the status icon at all in this case?

I would also be in favor of not showing the status in this case.

There are some small discrepancies in loading and error state:

  • For the details page Summary tab, we get a tiny, little loading bar. 🥺
    Screenshot 2024-01-16 at 11 38 24 AM
  • For the Configurations endpoints, when there's an error (blocked network request), we still see the "Endpoints:" text but no data, whereas the other endpoint locations surface an error. Can we make the behavior consistent?

Configurations:
Screenshot 2024-01-16 at 11 36 04 AM

Service Targets:
Screenshot 2024-01-16 at 11 36 35 AM

Details > Summary:
Screenshot 2024-01-16 at 11 36 51 AM

Landing:
Screenshot 2024-01-16 at 11 37 53 AM

@@ -0,0 +1,5 @@
---
"@linode/api-v4": Added
---
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to keep considering it an upcoming feature until it's in public beta.

In that case, should this changeset entry be under Upcoming Features rather than Added?

Comment on lines +23 to +24
expect(upStatusIcon).toHaveStyle({ backgroundColor: '#17cf73' });
expect(downStatusIcon).toHaveStyle({ backgroundColor: '#ca0813' });
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

@bnussman-akamai
Copy link
Member Author

also, more of a UX question, is it worth showing the status icon at all in this case?

I would also be in favor of not showing the status in this case.

I think I'm going to show endpoint status at all times because it's often inside a table and I don't think we need to add the complexity of conditionally hiding it.

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

because it's often inside a table and I don't think we need to add the complexity of conditionally hiding it.

As long as we don't receive user feedback that it's a reason for visual overwhelm in the table or is somehow confusing to users, I think keeping it is fine.

Thanks for the loading and error state updates - confirmed we no longer see a tiny loader on Summary and the endpoints on Configurations surface an error. ✅

Screenshot 2024-01-16 at 12 57 12 PM
Screenshot 2024-01-16 at 12 57 06 PM

@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Jan 16, 2024
@bnussman-akamai
Copy link
Member Author

Definitely will reevaluate the error states if they happen often or if we get feedback about them!

@bnussman-akamai bnussman-akamai merged commit be1d910 into linode:develop Jan 16, 2024
17 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACLB Relating to the Akamai Cloud Load Balancer Approved Multiple approvals and ready to merge! Ready for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants