Skip to content

[Metricbeat] Migrate Kubernetes state_node Metricset to use ReporterV2 interface#10962

Merged
sayden merged 7 commits intoelastic:masterfrom
sayden:migration/mb/reporterv2/kubernetes/state_node
Mar 11, 2019
Merged

[Metricbeat] Migrate Kubernetes state_node Metricset to use ReporterV2 interface#10962
sayden merged 7 commits intoelastic:masterfrom
sayden:migration/mb/reporterv2/kubernetes/state_node

Conversation

@sayden
Copy link
Copy Markdown
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 13:31
@sayden
Copy link
Copy Markdown
Contributor Author

sayden commented Feb 28, 2019

jenkins, test this

@sayden
Copy link
Copy Markdown
Contributor Author

sayden commented Mar 1, 2019

jenkins, test this again please

@sayden sayden force-pushed the migration/mb/reporterv2/kubernetes/state_node branch from 5caf770 to 9f3daa6 Compare March 1, 2019 11:53
@sayden sayden added the review label Mar 5, 2019
@sayden
Copy link
Copy Markdown
Contributor Author

sayden commented Mar 5, 2019

@ruflin
Copy link
Copy Markdown
Contributor

ruflin commented Mar 6, 2019

jenkins, test this

Copy link
Copy Markdown
Contributor

@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 good with merging this as is.

Overall it looks like a prime candidate for the new http test framework. I can take a stab at it as soon as this is merged.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would assume with v2 no _namespace field should be needed anymore?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I checked the code and it seems for prometheus modules namespace is used a bit different then I expected.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I checked the code and it seems for prometheus modules namespace is used a bit different then I expected.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we just add the event to the MetricSetFields here I think it will be reported under kubernetes.state_node but looking at the existing data.json it's under kubernetes.node:

  "kubernetes": {
    "node": {
      "cpu": {
        "allocatable": {
          "cores": 2
        },

We need to respect the namespace.

@sayden Can you verify that?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Isn't the namespace for this metricset always node?

Should be the same value as mapping.ExtraFields[mb.NamespaceKey]

@sayden sayden force-pushed the migration/mb/reporterv2/kubernetes/state_node branch from d277ee1 to 3c4a590 Compare March 8, 2019 15:14
@sayden sayden requested a review from a team as a code owner March 8, 2019 15:33
Copy link
Copy Markdown
Contributor

@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

@sayden sayden merged commit 265ab8b into elastic:master Mar 11, 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.

2 participants