Skip to content

Conversation

@showuon
Copy link
Contributor

@showuon showuon commented Sep 30, 2024

This PR adds the prometheus integration in recommendation-app. This is for local environment demo use. Will add the integration with Openshift cluster built-in Prometheus examples in a separate PR.

@showuon
Copy link
Contributor Author

showuon commented Sep 30, 2024

@katheris @tinaselenge , please take a look when available. Thanks.

Copy link
Member

@katheris katheris left a comment

Choose a reason for hiding this comment

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

I tried these and found a couple of updates were needed to work on Mac and also a few of the commands were missing -n flink.

More generally I wonder whether the instructions for deploying Prometheus should be in the recommendation-app README, or whether it would be better putting them in the top level README? Do you think we could write the instructions generically so if we added other examples in future it would explain how to use prometheus with any of the examples?

@showuon
Copy link
Contributor Author

showuon commented Oct 1, 2024

Thanks for the review @katheris !

I tried these and found a couple of updates were needed to work on Mac and also a few of the commands were missing -n flink.

Ah, right! I was testing in Ubuntu, and in my env, I set default namespace as flink, that's why I forgot to add -n flink. Sorry!

More generally I wonder whether the instructions for deploying Prometheus should be in the recommendation-app README, or whether it would be better putting them in the top level README? Do you think we could write the instructions generically so if we added other examples in future it would explain how to use prometheus with any of the examples?

Good point! I've moved the prometheus-install into root folder, and reference it in recommendation-app readme. Let me know if you have other suggestion. Thank you.

Copy link
Member

@katheris katheris left a comment

Choose a reason for hiding this comment

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

I think we should assume people will run the commands from the top level directory so I've made some suggestions around that and a wording tweak. Otherwise looks good to me, thanks Luke

@showuon
Copy link
Contributor Author

showuon commented Oct 1, 2024

@katheris , thanks for the review. I've addressed your comments. Besides, I also added the steps to integrate with Openshift prometheus in this commit: eb267dd . Please have another look when available. Thanks.

@katheris katheris self-requested a review October 1, 2024 13:14
@showuon
Copy link
Contributor Author

showuon commented Oct 2, 2024

@katheris @tinaselenge , thanks for the review. Addressed the comments. Thanks.

Copy link
Member

@katheris katheris left a comment

Choose a reason for hiding this comment

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

Thanks @showuon

Copy link
Contributor

@tinaselenge tinaselenge left a comment

Choose a reason for hiding this comment

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

Thanks Luke.

@showuon
Copy link
Contributor Author

showuon commented Oct 3, 2024

@kornys , do you have any other comment?

@kornys
Copy link
Member

kornys commented Oct 3, 2024

@showuon no, thank you for the fix and PR.

@SamBarker SamBarker merged commit 37fdbb9 into streamshub:main Oct 4, 2024
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.

5 participants