-
Notifications
You must be signed in to change notification settings - Fork 152
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
Feature flagging for exporting of Kanister prometheus metrics #2235
Conversation
Thanks for submitting this pull request 🎉. The team will review it soon and get back to you. If you haven't already, please take a moment to review our project contributing guideline and Code of Conduct document. |
helm/kanister-operator/values.yaml
Outdated
@@ -53,3 +53,5 @@ resources: | |||
# requests: | |||
# cpu: 100m | |||
# memory: 128Mi | |||
metrics: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add this as a sub-value under controller
above? Open to suggestions @viveksinghggits
Additionally, add a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Makes sense. I've made it as a sub-value under controller
. I've added a comment.
One more point - I'm maintaining two levels of hierarchy for the helm flags - like metrics.enabled
instead of metrics_enabled
because we may want to add more flags under metrics
hierarchy in the future if we consider adding labels for a new metric partially.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's the right way to do it 👍🏼
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we just want to set the env var in the pod as part of this PR? Do we not want to use it anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@viveksinghggits Yes. The feature flag will start getting used in the next set of PRs for the metrics we export.
Change Overview
Adding a new helm flag and environment variable for feature toggling of the new Kanister prometheus metrics integration.
controller.metrics.enabled
in the Values.yamlKANISTER_METRICS_ENABLED
in deployment.yaml to capture the metrics.enabled flag in our codebase.End goal: In order to switch on prometheus metrics, the end user can install the local helm chart with the following command:
helm install kanister ./helm/kanister-operator \ --create-namespace \ --namespace kanister \ --set image.repository=<your_registry>/<your_controller_image> \ --set image.tag=<your_image_tag> --controller.metrics.enabled=true
Pull request type
Please check the type of change your PR introduces:
Issues
Test Plan
After the above changes were made, the following tests were performed:
helm uninstall kanister -n kanister
to uninstall existing kanister deploymenthelm install kanister ./helm/kanister-operator --create-namespace --namespace kanister
to run the local helm chart for kanisterk get pods -n kanister
to identify the new kanister operator pod that's runningkubectl exec -n kanister -it <pod-name> -- /bin/bash
to exec into the pod.echo $KANISTER_METRICS_ENABLED
- the output should be a false.