Skip to content

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

Merged
odacremolbap merged 21 commits intoelastic:masterfrom
odacremolbap:task/ksm-service
Dec 10, 2019
Merged

[metricbeat]kubernetes kube-state-metrics service#14794
odacremolbap merged 21 commits intoelastic:masterfrom
odacremolbap:task/ksm-service

Conversation

@odacremolbap
Copy link
Copy Markdown
Contributor

@odacremolbap odacremolbap commented Nov 26, 2019

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
houndci-bot
houndci-bot reviewed Nov 26, 2019
View reviewed changes
Comment thread metricbeat/module/kubernetes/state_service/state_service.go
@odacremolbap odacremolbap changed the title [metricbeat]kubernets kube-state-metrics service [metricbeat]kubernetes kube-state-metrics service Nov 26, 2019
houndci-bot
houndci-bot reviewed Nov 26, 2019
View reviewed changes
Comment thread metricbeat/module/kubernetes/state_service/state_service_test.go
Comment thread metricbeat/module/kubernetes/state_service/state_service.go
@odacremolbap odacremolbap added the in progress Pull request is currently in progress. label Nov 26, 2019
@odacremolbap
Copy link
Copy Markdown
Contributor Author

odacremolbap commented Nov 27, 2019

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
Copy Markdown
Contributor Author

odacremolbap commented Nov 28, 2019

jenkins, test this

@odacremolbap
Copy link
Copy Markdown
Contributor Author

odacremolbap commented Nov 28, 2019

jenkins, test this please

@odacremolbap
Copy link
Copy Markdown
Contributor Author

odacremolbap commented Nov 28, 2019

issues do not seem related.
reviews are very welcomed.

@odacremolbap
Copy link
Copy Markdown
Contributor Author

odacremolbap commented Nov 29, 2019

jenkins test this

although issues are not related

odacremolbap and others added 3 commits December 5, 2019 12:54
@odacremolbap
remove non needed if block
59ecbb5
@odacremolbap
remove blank line
b77e363
@odacremolbap @ChrsMark
Update metricbeat/module/kubernetes/state_service/README.md
4239c8f 
udpate month with __expected__ merge date

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

odacremolbap 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
  • 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
Copy Markdown
Member

ChrsMark commented Dec 5, 2019 •
edited
Loading

@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
Copy Markdown
Contributor Author

odacremolbap commented Dec 5, 2019

+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!

ChrsMark
ChrsMark approved these changes Dec 5, 2019
View reviewed changes
Copy link
Copy Markdown
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
@odacremolbap
[metricbeat]kubernetes kube-state-metrics service (elastic#14794)
f59f47b 
* add kube-state-metrics service

(cherry picked from commit 3bb845f)
@odacremolbap odacremolbap mentioned this pull request Dec 18, 2019
Merged
odacremolbap pushed a commit that referenced this pull request Dec 24, 2019
Cherry-pick #14794 to 7.x: [metricbeat]kubernetes kube-state-metrics …
765fde2 
…service (#15179)

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

ChrsMark commented Jan 16, 2020

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

@odacremolbap
Copy link
Copy Markdown
Contributor Author

odacremolbap commented Jan 16, 2020

@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
Copy Markdown
Member

ChrsMark commented Jan 17, 2020

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.

This was referenced Jan 17, 2020
Merged
Merged
Merged
@ChrsMark ChrsMark added the test-plan-ok This PR passed manual testing label Jan 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChrsMark ChrsMark ChrsMark approved these changes

+1 more reviewer

@houndci-bot houndci-bot houndci-bot left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@ChrsMark ChrsMark

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@odacremolbap @ChrsMark @houndci-bot @andresrc