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

Feature: AWS CloudWatch Container Insights Addon #878

Merged
merged 8 commits into from
Dec 14, 2023

Conversation

5herlocked
Copy link
Contributor

Issue #, if available: #872

Description of changes:
Added container insights add-on using CoreAddOn.
Added documentation for container insights along with what it's doing.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Collaborator

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

@5herlocked Nice work on this new Addon. Some feedback.

  1. Doc can be improved aligning to enhanced CW features of the addon
  2. Managed Policy is missing
  3. CW logs functionality is missing
  4. mkdocs is missing
  5. adding to doc index is missing
  6. Are we not adding the blueprints stack. I would add and comment it.

lib/addons/cloud-watch-insights/index.ts Show resolved Hide resolved
lib/addons/cloud-watch-insights/index.ts Show resolved Hide resolved

const app = new cdk.App();

const addOn = new blueprints.addons.CloudWatchInsights();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we show additional different variants of usage like EBS, CW Logs etc?

lib/addons/cloud-watch-insights/index.ts Outdated Show resolved Hide resolved
docs/addons/aws-container-insights.md Outdated Show resolved Hide resolved
@elamaran11 elamaran11 linked an issue Nov 15, 2023 that may be closed by this pull request
1 task
@elamaran11
Copy link
Collaborator

@5herlocked Wanted to check if you need any help to make progress on this addon. We want to add this with next blueprints release.

@5herlocked
Copy link
Contributor Author

I'll have this completed in a few weeks.

@elamaran11
Copy link
Collaborator

I'll have this completed in a few weeks.

Could you plan to add this for upcoming blueprints release?

@5herlocked
Copy link
Contributor Author

Resolved most of the requested changes, still working on the documentation.

elamaran11
elamaran11 previously approved these changes Dec 12, 2023
Copy link
Collaborator

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

LGTM. @shapirov103 Please check from your end.

Copy link
Collaborator

@shapirov103 shapirov103 left a comment

Choose a reason for hiding this comment

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

@5herlocked

Looks great, however the way it stands now it will be very confusing for the customers to select the right addon, especially choosing between ContainerInsightsAddon and this addon.

So let's apply the following changes:

  1. deprecate the current container insights addon (e..g similar to what we did for calico)
  2. update documentation for the deprecated addon and specify that there will be no maintenace on it and customers should move over to the CloudWatchInsightsAddon.
  3. In the docs for this addon, also call out that it is replacing the current CI addon.

Copy link
Collaborator

@shapirov103 shapirov103 left a comment

Choose a reason for hiding this comment

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

Please see the recommended naming and deprecation changes, otherwise looks great.

Copy link
Collaborator

@elamaran11 elamaran11 left a comment

Choose a reason for hiding this comment

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

Nice work, Please make sure the screenshots are up-to-date after your testing.

Copy link
Collaborator

@shapirov103 shapirov103 left a comment

Choose a reason for hiding this comment

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

@5herlocked looks great!
@elamaran11 since this addon is not part of e2e test, have you tested it on your account?

@elamaran11
Copy link
Collaborator

@shapirov103 I was able to pull the fork and test, everything works fine. I can see CI in CW Console. Please proceed with e2e and merge.

Copy link
Collaborator

@shapirov103 shapirov103 left a comment

Choose a reason for hiding this comment

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

LGTM

@shapirov103 shapirov103 merged commit 1486357 into aws-quickstart:main Dec 14, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Containers Insights Enhanced Observability EKS Addon
3 participants