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] Add testdata for coredns module #11332

Merged
merged 1 commit into from
Mar 22, 2019

Conversation

ChrsMark
Copy link
Member

@ChrsMark ChrsMark commented Mar 20, 2019

As discussed on #10585, we need testdata support for CoreDNS (#10432) module.

This PR adds testdata support along with an extension on https://github.com/elastic/beats/blob/master/metricbeat/mb/testing/data/data_test.go so as to handle properly multiple docs.plain files and multiple data.json files in case there is conditional event generation like in this of CoreDNS.

The rationale here is "have many docs.plain and produce accordingly their data.json peers. To illustrate the point:

  1. docs.plain will produce data.json
  2. event1_docs.plain will produce data_event1.json
  3. event2_docs.plain will produce data_event2.json

With this I think we are identical to the previous state where again we had multiple data_*.json files.

Let me know what you think @ruflin.

@ChrsMark ChrsMark requested review from a team as code owners March 20, 2019 10:57
@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?

@ruflin
Copy link
Member

ruflin commented Mar 20, 2019

The part I don't fully understand yet is why we need multiple data.json files. The data.json file is for the docs, for example here: https://www.elastic.co/guide/en/beats/metricbeat/master/metricbeat-metricset-ceph-cluster_disk.html I understand that events can sometimes look a bit different but not sure if it provides much value in the docs as it's just an example and all the fields are still documented.

If we go down the path of showing more then one event, I would rather do it the same way like we do it for the expected files and write an array into the `data.

[
   {your events}
]

Like this all the logic around data.json still works and we don't need to add any additional magic to it.

@ruflin ruflin added module review Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Mar 20, 2019
@ChrsMark
Copy link
Member Author

ChrsMark commented Mar 20, 2019

The part I don't fully understand yet is why we need multiple data.json files. The data.json file is for the docs, for example here: https://www.elastic.co/guide/en/beats/metricbeat/master/metricbeat-metricset-ceph-cluster_disk.html I understand that events can sometimes look a bit different but not sure if it provides much value in the docs as it's just an example and all the fields are still documented.

TBH I'm not sure if we actually need multiple data.json files, I just thought it is the proper way to handle it, like in other modules as mentioned on #10585 (comment).
Would this, producing multiple data.json files, make the generated data more complete, or it is ok to just pick one event out of the many that are being produced during each FetchData iteration?

If we go down the path of showing more then one event, I would rather do it the same way like we do it for the expected files and write an array into the `data.

[
   {your events}
]

Like this all the logic around data.json still works and we don't need to add any additional magic to it.

Having all events in a list would be a solution, but no strong opinion on this in general. If you propose we go with it I'm fine, and the same if we decide to go with having only one event in the single data.json.
So what path do you think we should follow? 🙂

@ruflin
Copy link
Member

ruflin commented Mar 20, 2019

The conditional events were introduced before we had this new data generation mechanism. Before the data generation was based on a running service and it was not clear which event should be picked and it could have changed on each run. Now the outcome is 100% predicable, so the docs.plain file can be adjusted to make sure the correct event ends up in data.json.

For now I would stick to just keep the data.json but I definitively keep the idea around having an array in mind.

What we should still have for this PR is taking https://github.com/elastic/beats/blob/master/metricbeat/module/coredns/stats/_meta/test/metrics and put it in testdata/1.3.1.plain and generate the expected output. I also like your single prometheus entry files for dedicated testing but would remove the docs logic here. Pick the one you like most for the docs and put it into docs.plain.

@ChrsMark
Copy link
Member Author

Perfect @ruflin , Thank you for clarifying the conditional events part!

I will come back with a single data.json with an array inside, and I will transfer the old metrics to the new approach erasing old unnecessary files.

@ruflin
Copy link
Member

ruflin commented Mar 20, 2019

@ChrsMark There might a misunderstaing. I would not do the data.json with an array yet, all the other things, yes :-)

@ChrsMark
Copy link
Member Author

One more question :). Do you think we should remove system test since the check about field documentation is happening in data_test.go too?

@ruflin
Copy link
Member

ruflin commented Mar 20, 2019

Not yet. The system test executes the binary and checks if the config options actually work which we don't check here. But long term, perhaps we get there ;-)

@ChrsMark ChrsMark force-pushed the add-coredns-testdata branch 4 times, most recently from 0975057 to 074a441 Compare March 21, 2019 06:59
Signed-off-by: Chris <chrismarkou92@gmail.com>
Signed-off-by: Chris Mark <chrismarkou92@gmail.com>
@ChrsMark
Copy link
Member Author

This should be ok to review.

@ruflin ruflin merged commit 80c930c into elastic:master Mar 22, 2019
@ChrsMark ChrsMark deleted the add-coredns-testdata 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants