Skip to content
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 pod: tags to kubernetes_state status reason metrics #1884

Merged
merged 4 commits into from
Aug 22, 2018

Conversation

antoinepouille
Copy link
Contributor

What does this PR do?

Adding pod tags to kube_pod_container_status_waiting_reason and kube_pod_container_status_terminated_reason metrics

Motivation

Github issue #1418

Review checklist

  • PR has a meaningful title or PR has the no-changelog label attached
  • Feature or bugfix has tests
  • Git history is clean
  • If PR impacts documentation, docs team has been notified or an issue has been opened on the documentation repo

@@ -383,6 +383,8 @@ def kube_pod_container_status_waiting_reason(self, message, **kwargs):
tags.append(self._format_tag("kube_container_name", label.value))
elif label.name == "namespace":
tags.append(self._format_tag(label.name, label.value))
elif label.name == "pod":
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess kube_pod_container_status_waiting_reason() and kube_pod_container_status_terminated_reason() could use some refactoring to avoid so much duplicated code between them as only the metric name and whitelist differ from one to the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, makes sense! Looks better now

mfpierre
mfpierre previously approved these changes Jul 20, 2018
Copy link
Contributor

@mfpierre mfpierre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, if we could add some doc arround how the whitelist are important to not explose the cardinality of the metric for future reference, that would be great

@ofek ofek removed this from the 6.4.0 milestone Jul 26, 2018
gzussa
gzussa previously approved these changes Aug 8, 2018
Copy link
Contributor

@gzussa gzussa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antoinepouille antoinepouille dismissed stale reviews from gzussa and mfpierre via 8618f60 August 22, 2018 08:37
@antoinepouille antoinepouille force-pushed the antoine/ksm-adding-pod-tag-on-some-metrics branch from d59322f to 8618f60 Compare August 22, 2018 08:37
Copy link
Contributor

@gzussa gzussa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. Let's make sure CI pass before to merge tho.

@antoinepouille antoinepouille merged commit 196888f into master Aug 22, 2018
@antoinepouille antoinepouille deleted the antoine/ksm-adding-pod-tag-on-some-metrics branch August 22, 2018 08:52
@pdecat
Copy link
Contributor

pdecat commented Sep 14, 2018

Thanks!

Can you confirm which datadog-agent release will get this new feature?

Note: the actual metrics are named kubernetes_state.container.status_report.count.waiting and kubernetes_state.container.status_report.count.terminated, not kube_pod_container_status_waiting_reason and kube_pod_container_status_terminated_reason as stated in the PR description.

nmuesch pushed a commit that referenced this pull request Nov 1, 2018
* Adding pod tags to kube_pod_container_status_waiting_reason and kube_pod_container_status_terminated_reason metrics

* Refactoring metric logic for the two state reason metrics

* fix line lengths

* Adding comment regarding reason whitelist for cardinality
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants