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] Migrate Kubernetes state_replicaset Metricset to use ReporterV2 interface #10974

Conversation

sayden
Copy link
Contributor

@sayden sayden commented Feb 27, 2019

Refer to #10774 for more info

@sayden sayden added Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Feb 27, 2019
@sayden sayden self-assigned this Feb 27, 2019
@sayden sayden requested a review from a team as a code owner February 27, 2019 19:49
@sayden
Copy link
Contributor Author

sayden commented Feb 28, 2019

jenkins, test this

@sayden
Copy link
Contributor Author

sayden commented Mar 1, 2019

ruflin
ruflin previously approved these changes Mar 7, 2019
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. But what I'm asking myself now is if this is also potentially affected by the namespace issue?

@sayden sayden dismissed ruflin’s stale review March 8, 2019 10:02

Probably yes, I'll check and ask you to review again. Dismissing to avoid accidental "happy merge"

@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_replicaset branch from d5935d4 to 750b725 Compare March 13, 2019 10:51
@sayden sayden requested a review from a team as a code owner March 13, 2019 10:51
@sayden
Copy link
Contributor Author

sayden commented Mar 13, 2019

jenkins, test this please

@sayden sayden added review and removed in progress Pull request is currently in progress. labels Mar 13, 2019
@sayden sayden force-pushed the migration/mb/reporterv2/kubernetes/state_replicaset branch from 750b725 to 79a9a4d Compare April 2, 2019 13:58
@ruflin
Copy link
Member

ruflin commented Apr 3, 2019

jenkins, test this


var moduleFieldsMapStr common.MapStr
moduleFields, ok := event[mb.ModuleDataKey]
if ok {
Copy link
Member

Choose a reason for hiding this comment

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

Can you file a follow up issue so we improve the prometheus input so we can get rid of this. I think @odacremolbap worked already on some parts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't understand why some metricsets were using it and some not

Copy link
Member

Choose a reason for hiding this comment

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

Not sure I can follow your above comment. I'm ok with the current hack but want to make sure we follow up that is why the ask for an issue.

Copy link
Member

Choose a reason for hiding this comment

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

As soon as we have migrated all the new interface, I think we can remove the old testing methods. I wanted to keep them around for the migration to make sure we don't miss things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue opened #11624

@ruflin
Copy link
Member

ruflin commented Apr 3, 2019

jenkins, test this

@ruflin
Copy link
Member

ruflin commented Apr 3, 2019

I don't think the failures are related.

@sayden sayden merged commit 459c7f5 into elastic:master Apr 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Metricbeat Metricbeat review Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants