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

Add Prometheus default alerts and Grafana health dashboard #1635

Closed
wants to merge 1 commit into from

Conversation

Allex1
Copy link

@Allex1 Allex1 commented Nov 2, 2023

Addresses #858

Signed-off-by: Alex Birca <Allex21ro@yahoo.com>
@Allex1
Copy link
Author

Allex1 commented Nov 6, 2023

@zyyw pls review ?

@zyyw
Copy link
Collaborator

zyyw commented Nov 16, 2023

Hi @Allex1,

Thanks for contributing to harbor!
On the one hand, these two manifests introduced by this PR can be created manually and separately even if it's not merged in the harbor-helm project; on the other hand, this feature could enrich the harbor functionalities in terms of monitoring harbor application. Would you be able to continue supporting this feature if there are issues open regarding it?

Currently, we'll keep this PR open and collecting the demand/feedback of this feature from the community.
Thanks again!

@Allex1
Copy link
Author

Allex1 commented Nov 16, 2023

Hi @Allex1,

Thanks for contributing to harbor! On the one hand, these two manifests introduced by this PR can be created manually and separately even if it's not merged in the harbor-helm project; on the other hand, this feature could enrich the harbor functionalities in terms of monitoring harbor application. Would you be able to continue supporting this feature if there are issues open regarding it?

Currently, we'll keep this PR open and collecting the demand/feedback of this feature from the community. Thanks again!

@zyyw I agree these can be side-loaded (and that's what we currently do) but these are under feature flag so there are no braking changes.
Also by having them as part of the harbor-helm project could provide tremendous benefits from the community updates/changes as opposed to manage them internally .
Feedback gathering is fine I guess but that's why I linked to an older issue that was requesting this feature.
Thanks!

@Allex1
Copy link
Author

Allex1 commented Dec 22, 2023

@zyyw can we merge this ?

@Vad1mo
Copy link
Member

Vad1mo commented Dec 22, 2023

@Allex1 as @zyyw correctly mentioned, can you continue to maintain and expand this part in the helm chart?
If so, we can add this functionality to harbor, If not, we should add this to the Wiki as a starting point for users.

@Allex1
Copy link
Author

Allex1 commented Dec 22, 2023

@zyyw @Vad1mo I think this should be merged as a starting point for users nevertheless but I can also maintain it.
We had to sub chart this internally and add these resources separately in order to get some monitoring enabled on our stack. Having this ootb should be a good step forward.
Thanks

@MinerYang
Copy link
Collaborator

MinerYang commented Feb 5, 2024

We are now not an all-in-one services, and it is introduced the third-party Prometheus CRD which we have concerns about long term maintenance.

@Allex1
Copy link
Author

Allex1 commented Feb 5, 2024

@MinerYang This is under a feature flag and disabled by default so it's backwards compatible. This is a very common practice in multiple OSS helm charts. In case users want to enable this they will be required to deploy the PrometheusRule object themselves. I can add this comment in needed.

@volker-raschek
Copy link

volker-raschek commented Feb 9, 2024

Hello everyone, as a longtime maintainer of kubernetes environments, I can only endorse @Allex1's pull request.

I agree that this practice is common and would be very happy to see the changes again in a next release of the chart. This would not only reduce complexity for the environment I maintain, but also for anyone who would like to monitor their system without much effort.

@Allex1
Copy link
Author

Allex1 commented Feb 12, 2024

closing in favor of #1703

@Allex1 Allex1 closed this Feb 12, 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.

None yet

5 participants