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

Grab kube_node_info as kubernetes_state.node.count #4383

Merged
merged 2 commits into from
Aug 23, 2019
Merged

Conversation

therc
Copy link
Contributor

@therc therc commented Aug 15, 2019

What does this PR do?

It picks the kubelet's kube_node_info metric and exports it as kubernetes_state.node.count.

Motivation

Some people might want to graph kubelet counts grouped by kubelet_version or os_image, e.g. to track a Kubernetes upgrade over time.

Review checklist (to be filled by reviewers)

  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached
  • Feature or bugfix must have tests
  • Git history must be clean
  • If PR adds a configuration option, it must be added to the configuration file.

@therc therc requested review from a team as code owners August 15, 2019 22:02
therve
therve previously requested changes Aug 16, 2019
Copy link
Contributor

@therve therve left a comment

Choose a reason for hiding this comment

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

Thanks for the patch, looks reasonable. The existing tests need to be fixed, and it'd be nice to assert the new metric as well. We need to update metadata.csv, too.

Some people might want to graph kubelet counts grouped by kubelet_version or os_image, e.g. to track a Kubernetes upgrade over time.
@therc
Copy link
Contributor Author

therc commented Aug 16, 2019

I knew there was more. I fixed metadata (collecting the metric once a minute is plenty) and the tests. I didn't clean up unusual characters in the tags (spaces, colons, slashes), e.g. in os_image and container_runtime_version: should it be done?

@hkaj
Copy link
Member

hkaj commented Aug 19, 2019

Thanks @therc!

The interval in metadata.csv doesn't impact the collection freq, it's used for in-app interpolation between the counter and rate types. 60s is fine there, but the default (empty value) is fine, too.

As for unusual characters, this is handled automatically. Characters that are not valid in tag values are replaced with _ (eg: os_image:container-optimized_os_from_google).

cc @JulienBalestra until the feature we talked about lands I think you will find this useful.

hkaj
hkaj previously approved these changes Aug 19, 2019
@therc
Copy link
Contributor Author

therc commented Aug 20, 2019

Thanks, @hkaj. I saw that, as far as the tests are concerned, the invalid characters are preserved, but it makes sense for them to be sanitised downstream (in the statsd client, dd-agent or even on the server side, perhaps all).

@hkaj hkaj requested a review from therve August 20, 2019 12:51
@therc
Copy link
Contributor Author

therc commented Aug 20, 2019

@therve is this good to go?

@hkaj hkaj dismissed therve’s stale review August 23, 2019 17:11

changes were made

@hkaj hkaj merged commit 375063e into DataDog:master Aug 23, 2019
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.

3 participants