search

aws-observability / cdk-aws-observability-accelerator

CDK AWS Observability Accelerator
https://aws-observability.github.io/cdk-aws-observability-accelerator/
MIT No Attribution
140 stars 36 forks source link

Fixed with added rules configurator and rule files #65

Closed freschri closed 1 year ago

freschri commented 1 year ago

Note: some changes in here are also in https://github.com/aws-observability/cdk-aws-observability-accelerator/pull/76

elamaran11 commented 1 year ago

@freschri This is AWSome work, I tried to run this solution and im getting following error. I think you have missed on the dependencies to deploy this solution (which i believe you might have deployed manually while developing). Please check with terraform solution for parity. IMO prometheusservice.services.k8s.aws/v1alpha1 is not a right approach. Also Alerting is missing as you see in Terraform repo.

10:18:36 AM | CREATE_FAILED        | Custom::AWSCDK-EKS-KubernetesResource | AmpRulesConfigurat...0/Resource/Default
Received response status [FAILED] from custom resource. Message returned: Error: b'error: resource mapping not found for name: "rule-1" namespace: "ack-system" from "/tmp/manifest.y
aml": no matches for kind "RuleGroupsNamespace" in version "prometheusservice.services.k8s.aws/v1alpha1"\nensure CRDs are installed first\n'

You should be using CfnRuleGroupsNamespace to create this on AMP vs using this approach. You should use this construct to build the alerts and rules.

In order to move forward with following next steps :

  1. Fix all issues in this PR.
  2. Apply this solution to other open source patterns like existing cluster and graviton open source accelerator. Also make sure you test it
  3. Doc needs to be updated with metrics from other boards which is flowing with this change.
  4. Once you have it, we can test and merge it but this will be a temporary solution for few weeks until we have flux multi path which you are working on and also adding https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_aps.CfnRuleGroupsNamespace.html for rules and alerts to AMpAddon.
  5. Once we have Flux multi path support and AmpAddon in Blueprints next release, you can work making a permanent features in AmpAddon for alerts and rules for all 3 patterns and submit new PR.
elamaran11 commented 1 year ago

@freschri FYI, i tested this PR and everything works fine, i also see that i can see data in all dashboards. So once you fix the doc, we can review and merge it. Nice work!

elamaran11 commented 1 year ago

@freschri Do we have an update on this PR? This is an important feature which we should move forward to close soon. Also we need to close the blueprints AmpAddon PR which has this dependency

freschri commented 1 year ago

@freschri Do we have an update on this PR? This is an important feature which we should move forward to close soon. Also we need to close the blueprints AmpAddon PR which has this dependency

done updates; PR dependent on quickstart release

elamaran11 commented 1 year ago

/do-e2e-test single-new-eks-opensource-observability deploy

elamaran11 commented 1 year ago

/do-e2e-test single-new-eks-opensource-observability destroy

freschri commented 1 year ago

@freschri Looks good to me but you are missing to update existing-cluster OSS pattern for similar screenshots. Please update that. Also make sure when you pull from upstream

I guess no updates? existing oss says on main branch: Please see Single New EKS Open Source Observability Accelerator.

elamaran11 commented 1 year ago

@freschri Looks good to me but you are missing to update existing-cluster OSS pattern for similar screenshots. Please update that. Also make sure when you pull from upstream

I guess no updates? existing oss says on main branch: Please see Single New EKS Open Source Observability Accelerator.

Makes sense. LGTM.