-
Notifications
You must be signed in to change notification settings - Fork 813
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
[kubernetes_state] refactor gauge submission, fix container.restarts #3297
Conversation
3dd3590
to
b9869c9
Compare
b9869c9
to
5c0096e
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.
Also, need to check with Quentin why the CI sometimes fails on kazoo.client install
""" | ||
try: | ||
getattr(self, message.name)(message, **kwargs) | ||
if message.name in self.metric_to_gauge: | ||
self._submit_gauges(self.metric_to_gauge[message.name], message) |
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.
We loose the label argument if passed to process(). Should we keep it in _submit_gauges or just remove the corresponding code branch?
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 feel like it could be useful someday, but yeah let's kill it for now since it's not used.
'kube_deployment_status_replicas_unavailable': NAMESPACE + '.deployment.replicas_unavailable', | ||
'kube_deployment_status_replicas_updated': NAMESPACE + '.deployment.replicas_updated', | ||
'kube_deployment_spec_replicas': NAMESPACE + '.deployment.replicas_desired', | ||
'kube_pod_container_resource_requests_cpu_cores': NAMESPACE + '.container.cpu_requested', |
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.
The CI check fail on .container.cpu_requested, looks like the valid k8s metric name is : kube_pod_container_requested_cpu_cores
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.
That's actually a mistake in their doc, see: https://github.com/kubernetes/kube-state-metrics/blob/1751d86b25463c60234a37753024c7122d39cc26/pod.go#L85
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 think what makes the CI fail is that we need to merge the protobuf.bin
fixture, otherwise the metric doesn't exist. Tests pass locally when I copy the files.
5c0096e
to
7e6d507
Compare
Note: Please remember to review the Datadog Contribution Guidelines
if you have not yet done so.
What does this PR do?
container.restarts
metric typeMotivation
container.restarts is always
0
because we use the gauge value instead of counter value.Testing Guidelines
See DataDog/integrations-core#317