search

cdklabs / cdk-monitoring-constructs

Easy-to-use CDK constructs for monitoring your AWS infrastructure
https://constructs.dev/packages/cdk-monitoring-constructs
Apache License 2.0
483 stars 70 forks source link

[Refactor] Mix-up of NLB and ECS APIs #120

Open r0b0ji opened 2 years ago

r0b0ji commented 2 years ago

Currently, I see the ECS monitoring has been combined with NLB, ALB. For ex:

  1. monitorFargateService: monitors Fargate service, loadbalancer, targetGroup
  2. monitorSimpleFargateService: monitors Fargate service
  3. monitorFargateNetworkLoadBalancer: looks similar to monitorFargateService but here the loadbalancer and target group are passed separately and not from FargateService
  4. monitorApplicationLoadBalancer: similar to monitorFargateNetworkLoadBalancer but instead of NLB, it is for ALB
  5. monitorQueueProcessingFargateService:
  6. monitorQueueProcessingEc2Service

A better refactor and re-org in my opinion should be to split into specific parts and each part responsible for monitoring its own resource. This is my initial refactor proposal, but feel free to discuss and refactor as required.

Let's just have monitorSimpleFargateService and monitorSimpleEc2Service (we can even remove simple from name), where first monitors just FargateService and second monitors just Ec2Service. Then have monitorNetworkLoadBalancer, which monitors NetworkLoadBalancer and NetworkTarget Group, monitorApplicationLoadBalancer which monitors ApplicationLoadBalancer and ApplicationTargetGroup.

Now,

  1. remove /lib/monitoring/aws-ecs-patterns module or maybe keep it and then monitorNetworkLoadBalancedFargateService internally calls monitorFargateService, monitorNetworkLoadBalancer, monitorApplicationLoadBalancedFargateService calls monitorFargateService, monitorApplicationLoadBalancer, monitorQueueProcessingFargateService calls monitorFargateService and monitorSQS. Same for Ec2Service
  2. add a module /lib/monitoring/aws-ecs, this module contains monitorFargateService, monitorEc2Service

This is my mental model. I am happy to discuss more.

RichiCoder1 commented 2 years ago

Wanted to pipe in my two cents and support this. While it's nice to have the high level combined patterns, my expectation is it's ultimately instrumenting the primitives and underlying services.

voho commented 2 years ago

I agree that this is the right way to do it. But we have to handle this in a backwards-compatible way. Internally, I believe we already have a separation for service / load balancer / target group, but the API combines them.

mobob commented 1 month ago

Support this for sure, having a scan of the APIs and things seem a bit wonky.

Also going to pipe in as a new-ish user; the API's for Fargate seem to be somewhat incongruent with the other constructs APIs in as much as they accept a FargateService as opposed to an IFargateService... a quick pass over and my usual methods for manifesting references to my services for monitoring does not apply (storing ARNs in SSM). I understand i can pass the references directly around but CDK usually punishes me for that đź‘Š