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

[metricbeat]kubernetes kube-state-metrics service #14794

Merged
merged 21 commits into from
Dec 10, 2019

Conversation

odacremolbap
Copy link
Contributor

kubernetes Service metrics for kube-state-metrics.

refer to https://github.com/kubernetes/kube-state-metrics/blob/master/docs/service-metrics.md for the expected information.

@odacremolbap odacremolbap requested a review from a team as a code owner November 26, 2019 16:10
@odacremolbap odacremolbap changed the title [metricbeat]kubernets kube-state-metrics service [metricbeat]kubernetes kube-state-metrics service Nov 26, 2019
@odacremolbap odacremolbap added the in progress Pull request is currently in progress. label Nov 26, 2019
@odacremolbap
Copy link
Contributor Author

jenkins, test this

@odacremolbap odacremolbap added review containers Related to containers use case Metricbeat Metricbeat Team:Integrations Label for the Integrations team and removed in progress Pull request is currently in progress. labels Nov 27, 2019
@odacremolbap
Copy link
Contributor Author

jenkins, test this

@odacremolbap
Copy link
Contributor Author

jenkins, test this please

@odacremolbap
Copy link
Contributor Author

issues do not seem related.
reviews are very welcomed.

@odacremolbap
Copy link
Contributor Author

jenkins test this

although issues are not related

odacremolbap and others added 3 commits December 5, 2019 12:54
udpate month with __expected__ merge date

Co-Authored-By: Chris Mark <chrismarkou92@gmail.com>
@odacremolbap
Copy link
Contributor Author

@ChrsMark gracias for the review.

There are 2 of your concerns still open.

  • a comment at code, which I hope we agree on easily
  • allow for transformation on non-mapped labels. Non mapped labels are something new, they don't feel 100% native to current prometheus helper, since it has a "transform the event" processor that doesn't fit this scenario. My very personal opinion is that adding a second processor function is doable, but the overall design dives into quirkiness. I would like to consider all lessons learned from what we have found fetching kube-state-metrics to revisit the prometheus helper.

@ChrsMark
Copy link
Member

ChrsMark commented Dec 5, 2019

@ChrsMark gracias for the review.

There are 2 of your concerns still open.

  • a comment at code, which I hope we agree on easily

I'm ok with that just it would be good to add a small sample of input/expected output to make it more clear. I got this insight from the tests but would be nice to have it in the docstring too.

  • allow for transformation on non-mapped labels. Non mapped labels are something new, they don't feel 100% native to current prometheus helper, since it has a "transform the event" processor that doesn't fit this scenario. My very personal opinion is that adding a second processor function is doable, but the overall design dives into quirkiness. I would like to consider all lessons learned from what we have found fetching kube-state-metrics to revisit the prometheus helper.

+1 on skipping this for now. Do we have a tracking issue for stuff like that regarding the Prometheus helper where we could add this?

@odacremolbap
Copy link
Contributor Author

+1 on skipping this for now. Do we have a tracking issue for stuff like that regarding the Prometheus helper where we could add this?

we have some closed ones regarding the unified kube-state-metrics metricset and the changes the prometheus helper needed based on what we were finding. I will create a new issue including all that info so we can discuss.

thanks!

Copy link
Member

@ChrsMark ChrsMark 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 changes!

@odacremolbap odacremolbap merged commit 3bb845f into elastic:master Dec 10, 2019
@odacremolbap odacremolbap deleted the task/ksm-service branch December 10, 2019 09:19
@odacremolbap odacremolbap added the test-plan Add this PR to be manual test plan label Dec 10, 2019
odacremolbap pushed a commit to odacremolbap/beats that referenced this pull request Dec 18, 2019
* add kube-state-metrics service

(cherry picked from commit 3bb845f)
odacremolbap pushed a commit that referenced this pull request Dec 24, 2019
…service (#15179)

* [metricbeat]kubernetes kube-state-metrics service (#14794)
@ChrsMark ChrsMark self-assigned this Jan 16, 2020
@ChrsMark
Copy link
Member

I'm gonna manually test this. Any testing notes @odacremolbap to be added?

@odacremolbap
Copy link
Contributor Author

@ChrsMark nop,

I'm pretty sure you know how to handle this, but if someone needs a guidance testing, this should help

@ChrsMark
Copy link
Member

Tested this manually and works as expected. Only problem found is that the fields' mapping is not updated at https://github.com/elastic/beats/blob/master/metricbeat/module/kubernetes/state_service/_meta/fields.yml#L31 after the changes applied at #15190.

@odacremolbap I will set this one to test-plan-ok once the change in fields.yml is merged to 7.6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
containers Related to containers use case Metricbeat Metricbeat needs testing notes review Team:Integrations Label for the Integrations team test-plan Add this PR to be manual test plan test-plan-ok This PR passed manual testing v7.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants