-
Notifications
You must be signed in to change notification settings - Fork 211
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
Issue-581 Make metrics exposable based on helm variables #582
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #582 +/- ##
==========================================
+ Coverage 85.12% 85.91% +0.79%
==========================================
Files 12 12
Lines 1613 1633 +20
==========================================
+ Hits 1373 1403 +30
+ Misses 155 145 -10
Partials 85 85 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Josh Souza <[email protected]>
Has there been any testing done to prove that the modified chart allows exposing metrics for Prometheus? |
args: | ||
- -metrics-bind-address={{ .Values.metricsBindAddress }}:{{ int .Values.metricsPort }} |
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.
Does it have to be -metrics-bind-address
or --metrics-bind-address
?
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.
Based on:
Line 65 in 074d8b0
flag.StringVar(&metricsAddr, "metrics-bind-address", "127.0.0.1:6000", "The address the metric endpoint binds to.") |
And flag's documentation: https://pkg.go.dev/flag#hdr-Command_line_flag_syntax
My understanding is that it can be either. If there is a preference for double dashes, that's easy to adjust, but as
-disableFinalizer
below is a single dash, I went with single dash for consistency.
I utilized helm template when writing the code to ensure the appropriate adjustments to the k8s resources, and deploying it to a local kind cluster before/after with a second While I didn't explicitly test that Prometheus itself was working, this should be a sufficient test to verify that the changes are functioning as intended, and should allow metrics gathering by other pods in the cluster. |
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.
LGTM
Resolves: #581