-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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 a filter for non-running pods #5049
Conversation
ac5e9c7
to
f957a9e
Compare
@@ -1373,6 +1373,9 @@ type PodMetricsEndpoint struct { | |||
FollowRedirects *bool `json:"followRedirects,omitempty"` | |||
// Whether to enable HTTP2. | |||
EnableHttp2 *bool `json:"enableHttp2,omitempty"` | |||
// Drop pods that are not running. (Failed, Succeeded, Unknown). | |||
// More info: https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/#pod-phase | |||
FilterRunning *bool `json:"filterRunning,omitempty"` |
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.
@SuperQ do we want to comment that this is enabled by default?
Also wondering that since it is enabled by default does it make sense to be a pointer? Perhaps alternatively we can use the +kubebuilder:default
annotation?
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, I can do that.
As for the pointer, I wasn't quite sure how to make a bool true by default here.
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.
I'm not sure that +kubebuilder:default
will play nice with a boolean field (omitempty
might also come in the way) but it's worth checking.
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.
Yea, I was tinkering around with this a bit and couldn't get it to work. The code generator always wants to make omitempty bools into pointers.
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.
From the docs
// +kubebuilder:default
any
sets the default value for this field.
A default value will be accepted as any value valid for the field. Formatting for common types include: boolean: true, string: Cluster, numerical: 1.24, array: {1,2}, object: {policy: "delete"}). Defaults should be defined in pruned form, and only best-effort validation will be performed. Full validation of a default requires submission of the containing CRD to an apiserver.
but yeah, lets not get hung up on this, once we document the default value I think we are good.
f957a9e
to
273858f
Compare
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. We can follow up in another PR to do the same for ServiceMonitors.
@SuperQ can you run |
Add a boolean option to PodMetricsEndpoint to filter out non-running phases. * Enable by default. Fixes: prometheus-operator#4816 Signed-off-by: SuperQ <superq@gmail.com>
273858f
to
bb9b50f
Compare
Just adding a note here we have #4877 logged for service monitor :) |
Ever since prometheus-operator/prometheus-operator#5049, the relabel config workaround to ignore non-running pods is no longer needed. This commit cleans up the podmonitor to keep the code tidy. Signed-off-by: Lucas Servén Marín <lserven@gmail.com>
Description
Add a boolean option to PodMetricsEndpoint to filter out non-running phases.
Fixes: #4816
Type of change
What type of changes does your code introduce to the Prometheus operator? Put an
x
in the box that apply.CHANGE
(fix or feature that would cause existing functionality to not work as expected)FEATURE
(non-breaking change which adds functionality)BUGFIX
(non-breaking change which fixes an issue)ENHANCEMENT
(non-breaking change which improves existing functionality)NONE
(if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)Changelog entry