cdk-watchful icon indicating copy to clipboard operation
cdk-watchful copied to clipboard

feat: Add opensearch

Open ackjewtn opened this issue 2 years ago • 10 comments

Fixes https://github.com/cdklabs/cdk-watchful/issues/729

ackjewtn avatar Apr 08 '22 12:04 ackjewtn

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).

ayush987goyal avatar Apr 08 '22 13:04 ayush987goyal

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?

ackjewtn avatar Apr 08 '22 14:04 ackjewtn

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! :)

ayush987goyal avatar Apr 08 '22 14:04 ayush987goyal

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?

ackjewtn avatar Apr 08 '22 14:04 ackjewtn

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.

ayush987goyal avatar Apr 08 '22 15:04 ayush987goyal

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.

voho avatar Apr 08 '22 15:04 voho

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.

ackjewtn avatar Apr 08 '22 15:04 ackjewtn

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

Chriscbr avatar Apr 08 '22 16:04 Chriscbr

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

https://github.com/cdklabs/cdk-watchful/pull/801

ayush987goyal avatar Apr 08 '22 16:04 ayush987goyal

Whoops, I just missed it - done!

Chriscbr avatar Apr 08 '22 16:04 Chriscbr