-
Notifications
You must be signed in to change notification settings - Fork 16.8k
[stable/neo4j] Add Prometheus ServiceMonitor #21434
Conversation
Hi @duboisph. Thanks for your PR. I'm waiting for a helm member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/assign @unguiculus |
/auto-cc |
Also pinging @mneedham since you're this chart's maintainer :) |
Rebased & updated to be compatible with latest changes from #20942. |
/ok-to-test |
@zanhsieh what's next to get this merged? |
@duboisph @lachie83 |
Signed-off-by: Philip Dubois <hello@philipdubois.be>
Signed-off-by: Philip Dubois <hello@philipdubois.be>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: duboisph The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: Philip Dubois <hello@philipdubois.be>
Signed-off-by: Philip Dubois <hello@philipdubois.be>
Since the merge of #21896, I had to rebase again... @mattfarina, @mneedham, @moxious can you LGTM this please? |
@duboisph sorry it took me some time, there's been some helm chart reorganization going on over here. I have feedback for you. The PR is good, and the feature is valuable. However, due to this comment recently from @mattfarina, we're re-evaluating what we're going to do with this helm chart. This overall repo (not the neo4j chart) is going to be deprecated, and we're looking at moving the Neo4j chart to a new location here: https://github.com/neo4j-contrib/neo4j-helm What I'm planning right now is to do new development there, and deprecate this chart with a new PR shortly which will redirect users of this chart to go there instead. Now to the technical specifics of your PR -- adding prometheus configurability is a great idea, the value is clear. The ServiceMonitor part is problematic, because it uses an API which isn't generally available on your average kubernetes cluster by default. In my local testing, with for example vanilla GKE, using the service monitor does the right thing from the YAML perspective, but since the API/entity isn't available, it bounces off without error. I'd prefer not to include resources in this chart that aren't reasonably universal, as inclusion of these types of resources will pidgeonhole the chart into a narrower set of configurations. Given that -- I've already taken the essence of your idea (basically all of it except the servicemonitor) and merged it into the neo4j-helm repo above, along with a number of other features and goodies that aren't present in this repo. With all of that in mind -- I want to thank you for taking the time to put this together, it's clearly useful, but with the context of this repo getting deprecated and so on, I'd recommend closing this PR and not merging it. |
Hi @moxious, thanks for the follow-up. It's good to see the neo4j chart migrated to it's own repo and looks like there's already quite some improvements too. Configuring Neo4j and exposing the metrics port(s) was the main reason for the PR, a ServiceMonitor can always be added by other means. However I'd still love to see the inclusion of an (optional) ServiceMonitor to the chart at some point. It's true, it doesn't use vanilla API's, however the inclusion of the prometheus-operator and it's CRDs isn't that uncommon afaik and a lot of other charts have started to include ServiceMonitor and/or PrometheusRule resources recently. A quick search through this repo will show numerous results, including most common database systems :). In any case, I appreciate you getting back to me and I'm excited to start migrating our installations to Neo4j 4.0 and the new chart. This PR can be closed. |
Signed-off-by: Philip Dubois hello@philipdubois.be
Is this a new chart
No
What this PR does / why we need it:
This PR allows exposing the built-in Prometheus metrics. Optionally the chart can also create a ServiceMonitor
Which issue this PR fixes
Special notes for your reviewer:
Checklist
[stable/mychartname]
)