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

Coredns metricbeat module #10585

Merged
merged 24 commits into from
Mar 19, 2019
Merged

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Feb 5, 2019

This PR implements CoreDNS module described on #10432.

Co-Authored-By: Ioannis Androulidakis [email protected]

@ChrsMark ChrsMark requested review from a team as code owners February 5, 2019 19:11
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@ChrsMark ChrsMark force-pushed the coredns-metricbeat-module branch from 54a01b5 to 4617c08 Compare February 5, 2019 19:27
@ruflin ruflin added module review Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Feb 6, 2019
@ruflin ruflin mentioned this pull request Feb 6, 2019
9 tasks
metricbeat/module/coredns/_meta/fields.yml Outdated Show resolved Hide resolved
metricbeat/module/coredns/stats/stats.go Outdated Show resolved Hide resolved
@elasticcla
Copy link

Hi @ChrsMark, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

@ChrsMark
Copy link
Member Author

@ruflin , could you have a look on this, especially on fields declaration? We follow the apiserver's metricset approach (from Kuberentes module), however it was a little bit confusing the fact that every "fetch" is split into multiple events, according to the label matching. This leads into not being able to provide a sample data.json though.
Any thoughts on this?

@ruflin
Copy link
Contributor

ruflin commented Feb 12, 2019

For the data.json there is also a generator method that you can pass a condition, see here: https://github.com/elastic/beats/blob/master/metricbeat/mb/testing/data_generator.go#L66 This should help to generate a "nicer" data.json.

I think the label matching is expected but @jsoriano might be a better person to comment on this.

Copy link
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.

Looks already pretty good:

  • Changelog?
  • Add a system check for field documentation (I think it's off at the moment)
  • How is the data.json generated at the moment

Glad to see you could use the prometheus metricset wrapper.

metricbeat/module/coredns/stats/_meta/fields.yml Outdated Show resolved Hide resolved
metricbeat/module/coredns/stats/_meta/fields.yml Outdated Show resolved Hide resolved
@jsoriano
Copy link
Member

You can find examples of conditional events writting in rabbitmq and system socket metricsets.

@ChrsMark ChrsMark force-pushed the coredns-metricbeat-module branch 4 times, most recently from 2a9534a to 8dee1bf Compare February 19, 2019 12:37
@ioandr
Copy link
Contributor

ioandr commented Feb 19, 2019

Hey @ruflin, @jsoriano! We have added almost everything except for the unit-test. We will also need to curate the metric fields a bit in order to reach the desired results.

As far as the unit-test is concerned, is there any automated way to produce the metrics.expected used in the assertion?

@exekias might know more on this since he had done the same for apiserver in the kubernetes metricbeat module

@ioandr ioandr force-pushed the coredns-metricbeat-module branch from 8dee1bf to 679810a Compare February 22, 2019 08:58
@ioandr
Copy link
Contributor

ioandr commented Feb 22, 2019

Hey @ruflin, @jsoriano! We have added almost everything except for the unit-test. We will also need to curate the metric fields a bit in order to reach the desired results.

As far as the unit-test is concerned, is there any automated way to produce the metrics.expected used in the assertion?

@exekias might know more on this since he had done the same for apiserver in the kubernetes metricbeat module

We got around the issue of auto-generating the expected metrics by using the -update_expected flag in the go test command, as @ChrsMark indicated.

@ChrsMark ChrsMark changed the title [WIP] Coredns metricbeat module Coredns metricbeat module Feb 22, 2019
@ioandr
Copy link
Contributor

ioandr commented Feb 22, 2019

Hey, just a heads-up on the current state:

we have done most of the work described in #10432 and removed WIP from the PR title, so you might want to have a second review of this when you have time.

Let us know for any further changes, as we consider some minor commits to finalize the format of the metrics and the CoreDNS module.

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.

Thanks! this is looking quite good, I have added some comments, mainly about the fields mappings. Let me know what you think.

Also, do you happen to have a dashboard for this? 😇

CHANGELOG.asciidoc Outdated Show resolved Hide resolved
CHANGELOG.next.asciidoc Outdated Show resolved Hide resolved
metricbeat/module/coredns/_meta/Dockerfile Outdated Show resolved Hide resolved
@ChrsMark
Copy link
Member Author

Thanks for reviewing @jsoriano . We will iterate over the comments soon!

As far as the dashboard is concerned, sure thing in a follow up PR!☺️

@ChrsMark ChrsMark force-pushed the coredns-metricbeat-module branch 4 times, most recently from 894ae81 to 46af5c0 Compare February 26, 2019 09:41
ChrsMark and others added 14 commits March 19, 2019 10:46
Signed-off-by: Chris <[email protected]>

Co-Authored-By: Ioannis Androulidakis <[email protected]>
Signed-off-by: Chris Mark <[email protected]>
Signed-off-by: Ioannis Androulidakis <[email protected]>
Signed-off-by: Ioannis Androulidakis <[email protected]>
* Enable DO bit metrics with dnssec in DNS request

* Merge request and response size bytes metric

* Generate updated JSON files and fix integration test

* Generate updated metrics and metrics.expected files

Signed-off-by: Ioannis Androulidakis <[email protected]>
Signed-off-by: Chris <[email protected]>
Signed-off-by: Ioannis Androulidakis <[email protected]>
Signed-off-by: Chris Mark <[email protected]>
Signed-off-by: Chris Mark <[email protected]>
@ioandr ioandr force-pushed the coredns-metricbeat-module branch from e639a70 to 1c81377 Compare March 19, 2019 08:48
@ChrsMark ChrsMark force-pushed the coredns-metricbeat-module branch from 1c81377 to cc34863 Compare March 19, 2019 09:16
Signed-off-by: Chris Mark <[email protected]>
@jsoriano
Copy link
Member

jenkins, test this

@ioandr
Copy link
Contributor

ioandr commented Mar 19, 2019

@jsoriano thanks for triggering Jenkins again, we forced-pushed to rebase on the latest master

@ChrsMark
Copy link
Member Author

ChrsMark commented Mar 19, 2019

Failing test seems to be irrelevant. (wished for a greeny Jenkins 😞 , but not this time)

@jsoriano
Copy link
Member

Well, failing test looks irrelevant to this, yes, shipping it. Thanks for your work here!

@jsoriano jsoriano merged commit cb23a8e into elastic:master Mar 19, 2019
@ChrsMark
Copy link
Member Author

@jsoriano 👍 .
Would you like me to open an issue about testdata leftover? I could work on it and I have something in mind already but I would like to crosscheck the approach with one of the project's maintainers to ensure its validity. So, let me know.

@ruflin
Copy link
Contributor

ruflin commented Mar 20, 2019

@ChrsMark Would be great. You can also directly open a PR if you already have something in mind, no need for an issue.

@ChrsMark ChrsMark deleted the coredns-metricbeat-module branch August 28, 2019 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Metricbeat Metricbeat module review Team:Integrations Label for the Integrations team v7.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants