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.

Sign up for GitHub

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

Jump to bottom

[Metricbeat] Migrate Kubernetes state_container Metricset to use ReporterV2 interface #10858

Merged
merged 2 commits into from
Apr 1, 2019
Merged

[Metricbeat] Migrate Kubernetes state_container Metricset to use ReporterV2 interface #10858

merged 2 commits into from
Apr 1, 2019

Conversation

sayden
Copy link
Contributor

@sayden sayden commented Feb 20, 2019 •
edited by exekias
Loading

Refer to #10774 for more info

Found a really ugly dependency with Prometheus that needed to upgrade also the Prometheus helper so I have removed the dependency by duplicating code.

We can make a follow up PR to "clean" that. But right now I must keep focused on the migration

@sayden sayden added Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Feb 20, 2019
@sayden sayden self-assigned this Feb 20, 2019
@sayden sayden requested a review from a team as a code owner February 20, 2019 22:39
@sayden sayden mentioned this pull request Feb 20, 2019
Closed
62 tasks
@sayden
Copy link
Contributor Author

sayden commented Feb 21, 2019

jenkins, test this please

@sayden
Copy link
Contributor Author

sayden commented Feb 21, 2019

Error seems unrelaed in auditbeat

@sayden sayden changed the title Migrate Kubernetes state_container Metricset to use ReporterV2 interface [Metricbeat] Migrate Kubernetes state_container Metricset to use ReporterV2 interface Feb 22, 2019
@sayden sayden added the review label Feb 28, 2019
jsoriano
jsoriano previously requested changes Mar 1, 2019
View reviewed changes
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Not sure about the duplication of code, and could you also regenerate the data.json file?

metricbeat/module/kubernetes/state_container/state_container_test.go Outdated Show resolved Hide resolved
@sayden sayden force-pushed the migration/mb/reporterv2/kubernetes/state_container branch from 197a826 to ff9030f Compare March 4, 2019 12:27
@jsoriano jsoriano dismissed their stale review March 4, 2019 19:50

Changes addressed

@jsoriano
Copy link
Member

jsoriano commented Mar 4, 2019

Is it possible to regenerate data.json files?

@sayden
Copy link
Contributor Author

sayden commented Mar 6, 2019

As we talked in a different conversation, we'll leave data.json regeneration if they are not very quick wins. Also, now that #10648 is merged, maybe the generation will be in a different way

@sayden sayden force-pushed the migration/mb/reporterv2/kubernetes/state_container branch from 2b64d2b to 0c7423b Compare March 7, 2019 13:14
@sayden sayden requested a review from a team as a code owner March 7, 2019 14:52
@sayden sayden added in progress Pull request is currently in progress. and removed review labels Mar 8, 2019
@sayden sayden force-pushed the migration/mb/reporterv2/kubernetes/state_container branch from 095080a to c77efa1 Compare March 13, 2019 10:32
@sayden sayden requested a review from a team as a code owner March 13, 2019 10:32
ruflin
ruflin requested changes Mar 15, 2019
View reviewed changes
Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

I'm glad you already started to use the new data test framework. It shows well that we need to do work on the _module magic.

...dule/kubernetes/state_container/_meta/testdata/kube-state-metrics.v1.3.0.plain-expected.json Outdated Show resolved Hide resolved
@sayden sayden force-pushed the migration/mb/reporterv2/kubernetes/state_container branch from 27ad7c4 to fb87a61 Compare March 19, 2019 11:30
@ruflin
Copy link
Member

ruflin commented Mar 19, 2019

@sayden I think the CI failures are related.

@sayden
Copy link
Contributor Author

sayden commented Mar 19, 2019

jenkins, test this please

@sayden sayden force-pushed the migration/mb/reporterv2/kubernetes/state_container branch from 73719ab to e35f55e Compare March 19, 2019 21:51
@sayden
Copy link
Contributor Author

sayden commented Mar 25, 2019

jenkins, test this

@sayden sayden force-pushed the migration/mb/reporterv2/kubernetes/state_container branch from 69e436c to c8dbbe3 Compare March 26, 2019 12:52
@sayden sayden force-pushed the migration/mb/reporterv2/kubernetes/state_container branch from c8dbbe3 to e883bb0 Compare March 26, 2019 14:52
@sayden
Copy link
Contributor Author

sayden commented Mar 26, 2019

jenkins, test this

1 similar comment
@ruflin
Copy link
Member

ruflin commented Mar 27, 2019

jenkins, test this

ruflin
ruflin reviewed Mar 27, 2019
View reviewed changes
metricbeat/module/kubernetes/state_container/state_container.go

return events, err
var moduleFieldsMapStr common.MapStr
moduleFields, ok := event[mb.ModuleDataKey]
Copy link
Member

Choose a reason for hiding this comment

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

@odacremolbap I wonder if with your refactoring we will be able to remove this "trick".

ruflin
ruflin approved these changes Mar 27, 2019
View reviewed changes
Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

LGTM. Just to confirm: Metrics need to go under kubernetes.container?

@sayden
Copy link
Contributor Author

sayden commented Apr 1, 2019 •
edited
Loading

LGTM. Just to confirm: Metrics need to go under kubernetes.container?

Very good question, thanks for asking! In state_node we cut it to node, the same with state_deployment (we cut it to deployment).

I didn't understand this renaming very well and I was following the "approach" but, in this case, we also have a metricset called container so we also have there kubernetes.container.

In pre V2 version, metrics go under kubernetes.container but now I'm thinking if this was a bug / mistake.

@ruflin
Copy link
Member

ruflin commented Apr 1, 2019

For this PR, I would focus on having the metrics in the same place as before. So if they were in k8s.container before and are there again, we are good.

@sayden
Copy link
Contributor Author

sayden commented Apr 1, 2019

Ok, so merging. Do you want me to open an issue to discuss this collision?

@sayden sayden merged commit f81ac68 into elastic:master Apr 1, 2019
@ruflin
Copy link
Member

ruflin commented Apr 1, 2019

@sayden If you think there is some unexpected collision, please open one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruflin ruflin ruflin approved these changes

@jsoriano jsoriano jsoriano left review comments

Assignees

@sayden sayden

Labels
in progress Pull request is currently in progress. Metricbeat Metricbeat Team:Integrations Label for the Integrations team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sayden @jsoriano @ruflin