feat(elbv2): add metrics to INetworkLoadBalancer and IApplicationLoadBalancer #23853

edisongustavo · 2023-01-26T18:25:44Z

By moving the metrics methods to the INetworkLoadBalancer and IApplicationLoadBalancer interfaces it allows to create these metrics also for LBs that are imported via the fromXXX methods.

To create the metrics for LBs requires only the full name of the LB. This attribute is available at the constructs returned by the fromXXX methods.

To solve this problem I did:

Introduce a new interface for each LB type: INetworkLoadBalancerMetrics, IApplicationLoadBalancerMetrics
Create a concrete implementation for the new interfaces (1 for each): NetworkLoadBalancerMetrics and ApplicationLoadBalancerMetrics
Make each concrete implementation of each Load Balancer to also provide a metrics field. The concrete implementations of the load balancers are: ImportedApplicationLoadBalancer, LookedUpApplicationLoadBalancer, ApplicationLoadBalancer (and the same for the NLB classes).

I chose to create a new interface because code can be reused across the 3 concrete implementations of each Load Balancer. I deprecated the metricXXX() methods of each load balancer because I think it is cleaner to access metrics through the new metrics attribute/interface.

This task is a step in the direction to fix #10850, but I'd like to get feedback and merge this PR first before proceeding into the TargetGroup constructs.

PS: I'm learning Typescript, so please, tell me if I'm doing something wrong here.

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Construct Runtime Dependencies:

This PR adds new construct runtime dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2023-01-26T18:25:49Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

…Balancer By moving the metrics methods to the `INetworkLoadBalancer` and `IApplicationLoadBalancer` interfaces it allows to create these metrics also for LBs that are imported via the `fromXXX` methods. To create the metrics for LBs requires only the full name of the LB. This attribute is available at the constructs returned by the `fromXXX` methods. To solve this problem I did: - Introduce a new interface for each LB type: `INetworkLoadBalancerMetrics`, `IApplicationLoadBalancerMetrics` - Create a concrete implementation for the new interfaces (1 for each): `NetworkLoadBalancerMetrics` and `ApplicationLoadBalancerMetrics` - Make each concrete implementation of each Load Balancer to also provide a `metrics` field. The concrete implementations of the load balancers are: `ImportedApplicationLoadBalancer`, `LookedUpApplicationLoadBalancer`, `ApplicationLoadBalancer` (and the same for the NLB classes). I chose to create a new interface because code can be reused across the 3 concrete implementations of each Load Balancer. I deprecated the `metricXXX()` methods of each load balancer because I think it is cleaner to access metrics through the new `metrics` attribute/interface.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

edisongustavo · 2023-01-30T21:48:20Z

I've just discovered the flat design guidelines and I will change this PR to have a flat structure instead.

comcalvi · 2023-01-30T21:52:47Z

@edisongustavo what do you intend to change to be flat?

edisongustavo · 2023-01-30T22:15:51Z

Remove the Metrics interface. It will lead to more code repetition but it would follow the design guidelines.

The ImportedApplicationLoadBalancer and LookedUpApplicationLoadBalancer classes would have all the metricXXX() methods implemented by delegating them to the metrics field.

Or do you think that would be bad design? I particularly like what I did, but if the design guidelines are what they are, then so be it.

comcalvi · 2023-01-30T22:33:24Z

The design guidelines state

Do not introduce artificial nesting for props.

keyword here being artificial 🙂. Your changes do not violate this.

comcalvi

Nice work on this PR!

comcalvi · 2023-01-30T22:40:01Z

Is there anything we could change around the flat design guidelines to make them clearer? @edisongustavo

mergify · 2023-01-30T22:40:03Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

comcalvi · 2023-01-31T18:10:38Z

@Mergifyio refresh

mergify · 2023-01-31T18:11:40Z

refresh

✅ Pull request refreshed

Pull request has been modified.

mergify · 2023-01-31T18:50:36Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-01-31T19:42:18Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 362624d
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-01-31T19:42:37Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

edisongustavo · 2023-01-31T21:54:33Z

Is there anything we could change around the flat design guidelines to make them clearer? @comcalvi

I think they are well written, I just didn't pay enough attention to it 😅

…LBs (#23972) The `Arn.split()` method doesn't parse the `resourceName` correctly when it has multiple `/`, which is the case for the resources created by by the elbv2 API. I've also refactored the `integ.nlb-lookup.ts` test because it was not well written and I couldn't really deploy it with Cloudformation. The capability to create metrics from imported Load Balancers is new. It was introduced in #23853. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…tGroup (#23993) This PR follows the same conventions as #23853 By moving the metrics methods to the `INetworkTargetGroup` and `IApplicationTargetGroup` interfaces it allows to create these metrics also for Target Groups that are imported via the `fromTargetGroupAttributes()` method. To create the metrics for Target Groups requires (1) the full name of the Target Group and (2) the full name of the Load Balancer. For (1): it is readily available given that all imported Target Groups need to provide its ARN. For (2), it is an optional value, so the `.metrics` parameter will throw an error if it was not provided. To solve this problem I did: - Introduce a new interface for each TG type: `INetworkTargetGroupMetrics`, `IApplicationTargetGroupMetrics` - Create a concrete implementation for the new interfaces (1 for each): `NetworkTargetGroupMetrics` and `ApplicationTargetGroupMetrics` - Make each concrete implementation of each Load Balancer to also provide a `metrics` field. The concrete implementations of the load balancers are: `ImportedApplicationTargetGroup`, and `ApplicationLoadBalancer` (and the same for the NLB classes). I chose to create a new interface because code can be reused across the 3 concrete implementations of each Load Balancer. I deprecated the `metricXXX()` methods of each load balancer because I think it is cleaner to access metrics through the new `metrics` attribute/interface. There is a small **gotcha** here because the parameter of the `fromTargetGroupAttributes()` method that refers to the LB is: `loadBalancerArns`, which has its documentation as: > A Token representing the list of ARNs for the load balancer routing to this target group I'm not treating this parameter as a collection of ARNs, but as a single ARN. Also, I'm not treating it only as a token, but hardcoded ARNs can also be supplied, which "sort of" violates its interface. This attribute is weird though because Target Groups cannot have multiple Load Balancers as of today, although its documentation doesn't clearly express that is the case. fix: #10850 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Jan 26, 2023

aws-cdk-automation requested a review from a team January 26, 2023 18:25

aws-cdk-automation previously requested changes Jan 26, 2023

View reviewed changes

edisongustavo force-pushed the main branch from 3ba1aba to 5c772ed Compare January 26, 2023 18:32

edisongustavo changed the title ~~feat(elbv2): add metrics to INetworkLoadBalancer~~ chore(elbv2): add metrics to INetworkLoadBalancer Jan 26, 2023

edisongustavo force-pushed the main branch 2 times, most recently from 9f12266 to 7c080f3 Compare January 30, 2023 15:16

edisongustavo changed the title ~~chore(elbv2): add metrics to INetworkLoadBalancer~~ feat(elbv2): add metrics to INetworkLoadBalancer Jan 30, 2023

edisongustavo force-pushed the main branch from 7c080f3 to 40d7e4b Compare January 30, 2023 15:18

aws-cdk-automation previously requested changes Jan 30, 2023

View reviewed changes

edisongustavo force-pushed the main branch from 40d7e4b to f7c5357 Compare January 30, 2023 15:22

edisongustavo changed the title ~~feat(elbv2): add metrics to INetworkLoadBalancer~~ feat(elbv2): add metrics to INetworkLoadBalancer and IApplicationLoadBalancer Jan 30, 2023

edisongustavo force-pushed the main branch from f7c5357 to 3ff2ed6 Compare January 30, 2023 15:43

edisongustavo marked this pull request as ready for review January 30, 2023 16:37

comcalvi previously approved these changes Jan 30, 2023

View reviewed changes

Merge branch 'main' into main

0efedf7

comcalvi approved these changes Jan 31, 2023

View reviewed changes

Merge branch 'main' into main

362624d

mergify bot merged commit cb889bc into aws:main Jan 31, 2023

pahud mentioned this pull request Mar 1, 2023

aws-elasticloadbalancingv2: fromApplicationLoadBalancerAttributes broken since v2.64.0 #24397

Open

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(elbv2): add metrics to INetworkLoadBalancer and IApplicationLoadBalancer #23853

feat(elbv2): add metrics to INetworkLoadBalancer and IApplicationLoadBalancer #23853

edisongustavo commented Jan 26, 2023 •

edited

gitpod-io bot commented Jan 26, 2023

aws-cdk-automation left a comment

aws-cdk-automation left a comment

edisongustavo commented Jan 30, 2023

comcalvi commented Jan 30, 2023

edisongustavo commented Jan 30, 2023

comcalvi commented Jan 30, 2023

comcalvi left a comment

comcalvi commented Jan 30, 2023

mergify bot commented Jan 30, 2023

comcalvi commented Jan 31, 2023

mergify bot commented Jan 31, 2023

mergify bot commented Jan 31, 2023

aws-cdk-automation commented Jan 31, 2023

mergify bot commented Jan 31, 2023

edisongustavo commented Jan 31, 2023

feat(elbv2): add metrics to INetworkLoadBalancer and IApplicationLoadBalancer #23853

feat(elbv2): add metrics to INetworkLoadBalancer and IApplicationLoadBalancer #23853

Conversation

edisongustavo commented Jan 26, 2023 • edited

All Submissions:

Adding new Construct Runtime Dependencies:

New Features

gitpod-io bot commented Jan 26, 2023

aws-cdk-automation left a comment

Choose a reason for hiding this comment

aws-cdk-automation left a comment

Choose a reason for hiding this comment

edisongustavo commented Jan 30, 2023

comcalvi commented Jan 30, 2023

edisongustavo commented Jan 30, 2023

comcalvi commented Jan 30, 2023

comcalvi left a comment

Choose a reason for hiding this comment

comcalvi commented Jan 30, 2023

mergify bot commented Jan 30, 2023

comcalvi commented Jan 31, 2023

mergify bot commented Jan 31, 2023

✅ Pull request refreshed

mergify bot commented Jan 31, 2023

aws-cdk-automation commented Jan 31, 2023

AWS CodeBuild CI Report

mergify bot commented Jan 31, 2023

edisongustavo commented Jan 31, 2023

edisongustavo commented Jan 26, 2023 •

edited