Add kubernetes apiserver metricset by exekias · Pull Request #7059 · elastic/beats

exekias · 2018-05-09T15:50:06Z

This new metricset gathers metrics from Kubernetes API servers, giving visibility on number of requests and latency.

This change adds support for summary and histogram metric type to our Prometheus helper, histogram is used to retrieve latency info.

I also simplified the process of creating a new metricset based on this helper, to the point you only need this to create it and this to test it.

houndci-bot · 2018-05-09T15:50:27Z

metricbeat/helper/prometheus/module.go

exported function MetricSetBuilder should have comment or be unexported

houndci-bot · 2018-05-09T15:50:28Z

metricbeat/helper/prometheus/module.go

exported function MetricSetBuilder should have comment or be unexported

ruflin · 2018-05-11T09:51:04Z

metricbeat/module/kubernetes/apiserver/apiserver_test.go

This reminds me a lot of what @simitt was doing here with the approvals: https://github.com/elastic/apm-server/blob/master/tests/approvals.go I wanted to look into it to also do some comparison on json docs for Elasticsearch. Perhaps we can sync up on this.

Thats the approvals script that you can run from cmd line: https://github.com/elastic/apm-server/blob/master/tests/scripts/approvals.go

Would be great to put this in its own small library for more generic usage.

Agreed, we can discuss this and come up with a standardized way to deal with expected output in these tests, also pinging @jsoriano here, as he is working on some ideas to improve our testing.

The ideas I'm working on are more focused to improve and scale integration tests, but any abstraction like these ones are more than welcome of course :)

exekias · 2018-05-14T20:55:00Z

jenkins, retest this please

jsoriano

Pretty cool abstraction to collect prometheus metrics, surely lots things can be monitored this way.

jsoriano · 2018-05-15T07:39:22Z

metricbeat/helper/prometheus/module.go

What about using a flag for update as we do with go test -data?

jsoriano · 2018-05-15T07:43:36Z

metricbeat/module/kubernetes/apiserver/apiserver_test.go

The ideas I'm working on are more focused to improve and scale integration tests, but any abstraction like these ones are more than welcome of course :)

jsoriano · 2018-05-15T07:46:11Z

metricbeat/module/kubernetes/apiserver/apiserver.go

Use mb.Registry.MustAddMetricSet() here?

exekias · 2018-05-15T09:37:46Z

@jsoriano I've incorporated your suggestions, thanks!

exekias · 2018-05-15T11:32:57Z

jenkins, retest this please

ruflin

The main thing I'm worried is that a testing flag shows up in the production binary. Haven't tested if this is the case.

Seems with api-server we add one more thing to the kubenetes module which seems to be an other service?

ruflin · 2018-05-15T11:49:10Z

metricbeat/helper/prometheus/module.go

Is this only used for testing? If yes, should it really be registered here?

You are right, if it is defined here it will appear in the production binary. Testing code should be in a submodule to avoid that.

As we use cobra, common flags are not really "leaked" to the binary, I'm ok with moving this though

ruflin · 2018-05-15T11:49:44Z

metricbeat/helper/prometheus/module.go

Can happen in a later PR, but would be great to also use here the ReporterV2 interface.

ruflin · 2018-05-15T11:50:34Z

metricbeat/helper/prometheus/module.go

I'm thinking we should separate the testing parts and the module parts.

ruflin · 2018-05-15T11:53:01Z

metricbeat/module/kubernetes/apiserver/apiserver.go

That is probably the most compact metricset we have. I like it.

exekias · 2018-05-15T16:13:48Z

I converted the helper to use ReporterV2

exekias · 2018-05-15T22:33:04Z

jenkins, test this again

ruflin · 2018-05-16T12:00:07Z

@exekias Will need a rebase and a make update.

ruflin · 2018-05-16T12:06:53Z

metricbeat/helper/prometheus/ptest/ptest.go

I wonder if we should call these packages always testing as we do in some other places or if this is always and issue because there is a testing package.

Note: Just for discussion, no change in the PR needed.

I had the same thought, but as this is always used along with the Go testing package clash is guaranteed.

This already happens with the mb/testing package, we end up renaming it on import to mbtest, with the annoyance of breaking automatic imports.

There is another example in the latest changes on RabbitMQ module, all modules end up being renamed, but I think this is not so bad (https://github.com/elastic/beats/pull/7106/files#diff-7d67ada6eac5ae4109019e586c93d3a1R8)

I'm slightly in favour of having the same name for all test modules, though not sure if testing is the best option as it collides with the standard library.

For the module testing packages I think all must have the same name or structure, as we need to somehow modify the imports script to skip them. In general what i worry about directories in module is that they can always conflict with metricset names.

This component serves the Kubernetes API, used both internally by other components and externally to query/modify objects state (for instance by using kubectl)

Summaries are not aggregatable, and, while we don't have support to display histograms I still prefer to keep them over summaries.

exekias · 2018-05-16T15:07:54Z

jenkins, retest this please

exekias · 2018-05-17T08:56:59Z

jenkins, retest this please

This new metricset gathers metrics from Kubernetes API servers, giving visibility on number of requests and latency. This change adds support for `summary` and `histogram` metric type to our Prometheus helper, histogram is used to retrieve latency info. I also simplified the process of creating a new metricset based on this helper, to the point you only need [this](https://github.com/exekias/beats/blob/5f4568208ac273d9b759343b22f783184951a477/metricbeat/module/kubernetes/apiserver/apiserver.go) to create it and [this](https://github.com/exekias/beats/blob/5f4568208ac273d9b759343b22f783184951a477/metricbeat/module/kubernetes/apiserver/apiserver_test.go) to test it.

exekias added enhancement in progress Pull request is currently in progress. review containers Related to containers use case labels May 9, 2018

exekias mentioned this pull request May 9, 2018

[Meta Issue] Increase Kubernetes monitoring coverage #7058

Closed

23 tasks

houndci-bot reviewed May 9, 2018

View reviewed changes

metricbeat/helper/prometheus/module.go Outdated

Copy link
Copy Markdown

houndci-bot May 9, 2018

Choose a reason for hiding this comment

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

exported function MetricSetBuilder should have comment or be unexported

houndci-bot reviewed May 9, 2018

View reviewed changes

metricbeat/helper/prometheus/module.go Outdated

Copy link
Copy Markdown

houndci-bot May 9, 2018

Choose a reason for hiding this comment

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

exported function MetricSetBuilder should have comment or be unexported

exekias force-pushed the k8s-apiserver branch from b8ebe57 to d0b8b1d Compare May 9, 2018 16:45

ruflin reviewed May 11, 2018

View reviewed changes

exekias force-pushed the k8s-apiserver branch 2 times, most recently from b6ef84d to a64eb74 Compare May 14, 2018 18:07

exekias removed the in progress Pull request is currently in progress. label May 14, 2018

jsoriano reviewed May 15, 2018

View reviewed changes

exekias force-pushed the k8s-apiserver branch from 4643e12 to ce6ba2c Compare May 15, 2018 09:48

jsoriano approved these changes May 15, 2018

View reviewed changes

ruflin reviewed May 15, 2018

View reviewed changes

exekias force-pushed the k8s-apiserver branch 2 times, most recently from 3594513 to 52b2c0b Compare May 15, 2018 16:35

ruflin reviewed May 16, 2018

View reviewed changes

Carlos Pérez-Aradros Herce added 5 commits May 16, 2018 14:58

Add Kubernetes API server metricset

a31af5b

This component serves the Kubernetes API, used both internally by other components and externally to query/modify objects state (for instance by using kubectl)

Add summary support to Prometheus helper

5776720

Add latency summary metrics

e439d37

Fix comments

4682e58

Add fields.yml

6f95ed6

Carlos Pérez-Aradros Herce added 10 commits May 16, 2018 14:58

Add config

a026c60

Use histogram instead of summaries

09f3e9a

Summaries are not aggregatable, and, while we don't have support to display histograms I still prefer to keep them over summaries.

Add dashboard

a429995

Fix reference format

81a875e

Add -update_expected flag

ffff3d5

Use MustAddMetricSet

0ecdd65

Update fields.go

06f8248

Add CHANGELOG

d270833

Move testing code to its own package

5a4a97a

Convert to MetricsReporterV2

ffa4308

exekias force-pushed the k8s-apiserver branch from 52b2c0b to ffa4308 Compare May 16, 2018 12:59

jsoriano mentioned this pull request May 16, 2018

Move common RabbitMQ testing code to its own package #7106

Merged

ruflin merged commit b1f1b5f into elastic:master May 17, 2018

This was referenced Jan 30, 2019

Add support to monitor kubernetes kube-proxy #10431

Closed

Add support to monitor CoreDNS #10432

Closed

ioandr mentioned this pull request Feb 19, 2019

Coredns metricbeat module #10585

Merged

Conversation

exekias commented May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exekias commented May 14, 2018

Uh oh!

jsoriano left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsoriano May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exekias commented May 15, 2018

Uh oh!

exekias commented May 15, 2018

Uh oh!

ruflin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exekias commented May 15, 2018

Uh oh!

exekias commented May 15, 2018

Uh oh!

ruflin commented May 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exekias commented May 16, 2018

Uh oh!

exekias commented May 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

exekias commented May 9, 2018 •

edited

Loading

jsoriano May 15, 2018 •

edited

Loading