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: Add opensearch #800

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

ackjewtn
Copy link

@ackjewtn ackjewtn commented Apr 8, 2022

Fixes #729

@ackjewtn ackjewtn changed the title Add opensearch feat: Add opensearch Apr 8, 2022
@ayush987goyal
Copy link
Member

Hey @ackjewtn , thanks for the contribution. Just wondering if you had a chance to checkout https://github.com/cdklabs/cdk-monitoring-constructs which is another solution for monitoring that already has OpenSearch support (among many others).

@ackjewtn
Copy link
Author

ackjewtn commented Apr 8, 2022

Hello @ayush987goyal

No, that project was unknown to me, and seems to be more complete than cdk-watchful. Is cdk-watchful going to be replaced by https://github.com/cdklabs/cdk-monitoring-constructs?

@ayush987goyal
Copy link
Member

That would be the goal eventually. We are yet to figure out a migration path for the same. I would suggest you give it a try and let us know how you find it! :)

@ackjewtn
Copy link
Author

ackjewtn commented Apr 8, 2022

Already on it, it looks quite good. :) The sad part is that i spent quite some hours on this PR. Would probably be a good idea to link to cdk-monitoring-constructs in the README and inform about the future plans.

Not sure if you see any point in merging my code either?

@ayush987goyal
Copy link
Member

ayush987goyal commented Apr 8, 2022

I am really sorry about the same and I can see the amount of work that went into this PR. We will still keep it open to see if someone from the team wants to review and get it to merge. If you are blocked on it, would highly recommend to try to onboard to cdk-monitoring-constructs for long term support and features.

Also, agree on the point of updating the README to point users to the cdk-monitoring-constructs. We are currently also working on a potential migration guide which people could use to easily migrate to it before we do that.

@voho
Copy link
Contributor

voho commented Apr 8, 2022

Hi! Technically, there is no blocker for this PR to get merged and we can make it happen. The new monitoring library is still quite fresh on GitHub and we are figuring out how to proceed with the migration. In theory, you could check our current implementation of OpenSearch monitoring and see if there are some bits and pieces in your CR that could be reused, for example, metrics, useful widgets, anything. We will try to come to a conclusion quickly and update the repo with some banner, etc, to notify users of the library. Sorry about the churn.

@ackjewtn
Copy link
Author

ackjewtn commented Apr 8, 2022

No worries, things are changing all the time. :) As i said, it looks more complete, future-proof and extensible.

I'll check it out and see if anything could be improved. Most of my time on this PR went into implementing https://docs.aws.amazon.com/opensearch-service/latest/developerguide/cloudwatch-alarms.html and dynamically adding alarms/GraphWidgets based on the Domain/Cluster configuration. (Amount of nodes in the cluster, different types of nodes etc).

Side note: I saw that the build step failed previously, probably because i missed to commit the snapshot file.

@Chriscbr
Copy link

Chriscbr commented Apr 8, 2022

@ayush987goyal Would you like to update the README for cdk-watchful to reference the monitoring constructs project as another solution? :)

@ayush987goyal
Copy link
Member

@Chriscbr We already have a PR for the same from @voho. Could you please merge it in (and/or suggest edits)?

#801

@Chriscbr
Copy link

Chriscbr commented Apr 8, 2022

Whoops, I just missed it - done!

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.

Support Amazon OpenSearch
4 participants