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.

Sign up for GitHub

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

Jump to bottom

[Metricbeat] Testing: Allow use of different directory to test data instead of the expected one #34467

Merged
merged 25 commits into from
Feb 20, 2023
Merged

[Metricbeat] Testing: Allow use of different directory to test data instead of the expected one #34467

Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
c0d45a0
Allow change of directory.
constanca-m Feb 2, 2023
eaa103d
Update CHANGELOG.next.asciidoc
constanca-m Feb 2, 2023
150df6b
Fix testdata testing function
constanca-m Feb 3, 2023
2693f39
Do not test data.json
constanca-m Feb 3, 2023
7eeeb91
Fix collector_test.go
constanca-m Feb 6, 2023
9d7e4c9
Fix path comment.
constanca-m Feb 6, 2023
b1ef5ad
Merge branch 'elastic:main' into refactor-test-data-mb
constanca-m Feb 9, 2023
5b7a2cd
Revert collector changes.
constanca-m Feb 9, 2023
b54289e
Move difference to the correct changelog.
constanca-m Feb 9, 2023
8682c37
Add comment to function.
constanca-m Feb 9, 2023
129baac
Fix comment to function.
constanca-m Feb 9, 2023
e5890e2
Fix collector files.
constanca-m Feb 9, 2023
e9fbddd
Fix collector files.
constanca-m Feb 9, 2023
b340c6c
Fix collector files.
constanca-m Feb 9, 2023
9d755ea
Fix collector files.
constanca-m Feb 9, 2023
b6b1a2d
Fix collector files.
constanca-m Feb 9, 2023
e0c13cc
Fix collector files.
constanca-m Feb 9, 2023
c36ebd9
Update optional string to mandatory string.
constanca-m Feb 9, 2023
5f512ed
Merge branch 'elastic:main' into refactor-test-data-mb
constanca-m Feb 9, 2023
a8aebff
Remove misplaced folder.
constanca-m Feb 9, 2023
e5be71c
Update README.md
constanca-m Feb 9, 2023
79f4804
Update README.md
constanca-m Feb 9, 2023
22abe62
Set default data.json location
constanca-m Feb 14, 2023
5a8fa8c
Update readme
constanca-m Feb 14, 2023
febd2e2
Update readme
constanca-m Feb 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG-developer.next.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,7 @@ The list below covers the major changes between 7.0.0-rc2 and main only.
- Add an option to disable event normalization when creating a `beat.Client`. {pull}33657[33657]
- Add the file path of the instance lock on the error when it's is already locked {pull}33788[33788]
- Add DropFields processor to js API {pull}33458[33458]
- Add support for different folders when testing data {pull}34467[34467]

==== Deprecated

Expand Down
2 changes: 1 addition & 1 deletion metricbeat/mb/testing/data/data_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func TestAll(t *testing.T) {

} else {
config := mbtest.ReadDataConfig(t, f)
mbtest.TestDataFilesWithConfig(t, moduleName, metricSetName, config)
mbtest.TestDataFilesWithConfig(t, moduleName, metricSetName, config, getModulesPath())
}
})
}
Expand Down
65 changes: 52 additions & 13 deletions metricbeat/mb/testing/testdata.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import (
const (
expectedExtension = "-expected.json"
applicationJson = "application/json"
expectedFolder = "_meta/testdata"
)

// DataConfig is the configuration for testdata tests
Expand All @@ -64,7 +65,7 @@ const (
// A test will be run for each file with the `plain` extension in the same directory
// where a file with this configuration is placed.
type DataConfig struct {
// Path is the directory containing this configuration
// Path is the directory containing the read files
Path string

// WritePath is the path where to write the generated files
Expand Down Expand Up @@ -111,8 +112,8 @@ type DataConfig struct {

func defaultDataConfig() DataConfig {
return DataConfig{
Path: ".",
WritePath: ".",
Path: expectedFolder,
WritePath: expectedFolder,
Suffix: "json",
ContentType: applicationJson,
}
Expand All @@ -122,8 +123,7 @@ func defaultDataConfig() DataConfig {
func ReadDataConfig(t *testing.T, f string) DataConfig {
t.Helper()
config := defaultDataConfig()
config.Path = filepath.Dir(f)
config.WritePath = filepath.Dir(config.Path)

configFile, err := ioutil.ReadFile(f)
if err != nil {
t.Fatalf("failed to read '%s': %v", f, err)
Expand All @@ -139,25 +139,41 @@ func ReadDataConfig(t *testing.T, f string) DataConfig {
// from the usual path
func TestDataConfig(t *testing.T) DataConfig {
t.Helper()
return ReadDataConfig(t, "_meta/testdata/config.yml")
f := filepath.Join(expectedFolder, "config.yml")
return ReadDataConfig(t, f)
}

// TestDataFiles run tests with config from the usual path (`_meta/testdata`)
func TestDataFiles(t *testing.T, module, metricSet string) {
t.Helper()
config := TestDataConfig(t)
TestDataFilesWithConfig(t, module, metricSet, config)
TestDataFilesWithConfig(t, module, metricSet, config, "")
}

// TestDataFilesWithConfig run tests for a testdata config
func TestDataFilesWithConfig(t *testing.T, module, metricSet string, config DataConfig) {
func TestDataFilesWithConfig(t *testing.T, module, metricSet string, config DataConfig, testPrefix string) {
t.Helper()
ff, err := filepath.Glob(filepath.Join(config.Path, "*."+config.Suffix))

// the location of the read files
location := filepath.Join(config.Path, "*."+config.Suffix)

// if this function was called from data_test.go then the testPrefix is defined and not the default empty string
if testPrefix != "" {
// the prefix for read and write files should be ../../../module/moduleName/metricsetName
prefix := filepath.Join(testPrefix, module, metricSet)
location = filepath.Join(prefix, location)

// the prefix needs to be appended to the path of the expected files and the original files
config.WritePath = filepath.Join(prefix, config.WritePath)
ChrsMark marked this conversation as resolved.
Show resolved Hide resolved
config.Path = filepath.Join(prefix, config.Path)
}
ff, err := filepath.Glob(location)

if err != nil {
t.Fatal(err)
}
if len(ff) == 0 {
t.Fatalf("test path with config but without data files: %s", config.Path)
t.Fatalf("read test path without data files: %s", config.Path)
}

var files []string
Expand Down Expand Up @@ -193,6 +209,26 @@ func TestMetricsetFieldsDocumented(t *testing.T, metricSet mb.MetricSet, events
}

func runTest(t *testing.T, file string, module, metricSetName string, config DataConfig) {
filename := filepath.Base(file)
constanca-m marked this conversation as resolved.
Show resolved Hide resolved
/*
If the expected suffix is '.json' we need to exclude the data.json file, since
by the end of this function we may create a data.json file if there is a docs file with the config suffix:
if strings.HasSuffix(file, "docs."+config.Suffix) {
writeDataJSON(t, data[0], filepath.Join(config.WritePath, "data.json"))
}
Since the expected file name is obtained through filename + expectedExtension, we could end up testing files like:
Metrics file: data.json
Expected metrics file: data.json-expected.json
If the config extension is '.json'.
This is not possible, since running go -test data does not produce an expected file for data.json files. This is why
we need to exclude this file from the tests.
*/
if filename == "data.json" {
return
}

t.Logf("Testing %s file\n", file)

// starts a server serving the given file under the given url
s := server(t, file, config.URL, config.ContentType)
defer s.Close()
Expand Down Expand Up @@ -242,22 +278,25 @@ func runTest(t *testing.T, file string, module, metricSetName string, config Dat
err, module, metricSetName)
}

expectedFile := filepath.Join(config.WritePath, filename+expectedExtension)
ChrsMark marked this conversation as resolved.
Show resolved Hide resolved

// Overwrites the golden files if run with -generate
if *flags.DataFlag {
outputIndented, err := json.MarshalIndent(&data, "", " ")
if err != nil {
t.Fatal(err)
}
if err = ioutil.WriteFile(file+expectedExtension, outputIndented, 0644); err != nil {
if err = ioutil.WriteFile(expectedFile, outputIndented, 0644); err != nil {
t.Fatal(err)
}
}

// Read expected file
expected, err := ioutil.ReadFile(file + expectedExtension)
expected, err := ioutil.ReadFile(expectedFile)
if err != nil {
t.Fatalf("could not read file: %s", err)
}
t.Logf("Expected %s file\n", expectedFile)

expectedMap := []mapstr.M{}
if err := json.Unmarshal(expected, &expectedMap); err != nil {
Expand Down Expand Up @@ -369,7 +408,7 @@ func documentedFieldCheck(foundKeys mapstr.M, knownKeys map[string]interface{},
splits := strings.Split(foundKey, ".")
found := false
for pos := 1; pos < len(splits)-1; pos++ {
key := strings.Join(splits[0:pos], ".") + ".*." + strings.Join(splits[pos+1:len(splits)], ".")
key := strings.Join(splits[0:pos], ".") + ".*." + strings.Join(splits[pos+1:], ".")
if _, ok := knownKeys[key]; ok {
found = true
break
Expand Down
11 changes: 7 additions & 4 deletions metricbeat/module/openmetrics/collector/_meta/data.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,18 @@
"period": 10000
},
"openmetrics": {
"help": "Total number of connections closed that were made to the listener of a given name.",
"labels": {
"job": "openmetrics"
"job": "openmetrics",
"listener_name": "http"
},
"metrics": {
"up": 1
}
"net_conntrack_listener_conn_closed_total": 0
},
"type": "counter"
},
"service": {
"address": "127.0.0.1:55555",
"type": "openmetrics"
}
}
}
2 changes: 2 additions & 0 deletions metricbeat/module/openmetrics/collector/_meta/samelabeltestdata/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ type: http
url: "/metrics"
content_type: "application/openmetrics-text"
suffix: plain
path: "_meta/samelabeltestdata"
writepath: "_meta/samelabeltestdata"
remove_fields_from_comparison: ["openmetrics.labels.instance"]
module:
enable_exemplars: false
Expand Down
24 changes: 24 additions & 0 deletions metricbeat/module/openmetrics/collector/_meta/samelabeltestdata/data.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the reason of generation data.json now? for both samelabeltestdata and testdata?

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 don't know, that was already in the code. It was added because I ran the tests locally.

Copy link
Contributor

@tetianakravchenko tetianakravchenko Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

that was already in the code.

what do you mean? those files were not in samelabeltestdata and testdata folders before your changes. And as I see in other modules this file is not present (example - https://github.com/elastic/beats/tree/main/metricbeat/module/prometheus/collector/_meta/testdata, similar files structure with the openmetrics module)

Copy link
Contributor Author

@constanca-m constanca-m Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, should have been more clear. There are these lines to create that file. Since I ran the tests locally, the files were created.

Copy link
Contributor

Choose a reason for hiding this comment

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

if running tests locally from main branch (as described in this doc) data.json are not created on the metricsets level, please double check why it gets created after your changes

Copy link
Contributor

Choose a reason for hiding this comment

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

it will be a breaking change for our documentation, it should be changed

@elastic/elastic-agent-data-plane could you please review?

Copy link
Member

Choose a reason for hiding this comment

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

If I am interpreting this correctly, we are proposing to stop updating the data.json file that is used to render example events published in our official documentation? For example https://www.elastic.co/guide/en/beats/metricbeat/current/metricbeat-metricset-redis-info.html that was already linked?

If this understanding is correct then,

  1. We should keep the data.json field up to date. Users are likely relying on these pages to know what fields exist in the events for querying and processing. Removing this isn't helping our users, even if it may be convenient for us.
  2. If we have discovered that data.json is not being maintained or generated consistently, then that seems like a bug causing the documentation to be out date and should be fixed.

Copy link
Contributor Author

@constanca-m constanca-m Feb 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If I am interpreting this correctly, we are proposing to stop updating the data.json file that is used to render example events published in our official documentation? For example https://www.elastic.co/guide/en/beats/metricbeat/current/metricbeat-metricset-redis-info.html that was already linked?

Yes, you're correct. But the way we are referencing these files is wrong. And the one that you linked is not affected by this, because there are no files referenced there generated by this function. But going back to the ones that are actually affected... We can see a good example in this changed module openmetrics. We have two new data.json files: one for samelabeltestdata (created by running this test) and another for testdata (created by running this test). Previously, running these two tests would result in just one data.json file (running one function after the other, is overwriting what the other did), misplaced in this directory.

So if:

  1. We want to always create just one data.json file and never have the possibility to create more than one for different configs, this PR is breaking that.
  2. If we want to fix it, this PR is needed for that, but that requires changing the path for the data.json files in some of the modules. If this is not done, the referenced data.json files in the docs will stop being updated.

I would say that as it is now, this is a bug and we should fix it.

But if we want to keep doing 1., then updating it would be very easy as all the docs files are auto generated.

Copy link
Member

Choose a reason for hiding this comment

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

I went and looked at how the data.json files are actually pulled into our documentation. Picking the Redis info one that is already referenced (arbitrarily, I know it isn't affected) the data.json file is included in the docs with this asciidoc syntax:

beats/metricbeat/docs/modules/redis/info.asciidoc

Lines 16 to 27 in 0d90255

==== Fields
For a description of each field in the metricset, see the
<<exported-fields-redis,exported fields>> section.
Here is an example document generated by this metricset:
[source,json]
----
include::../../../module/redis/info/_meta/data.json[]
----
:edit_url!:

You can see that we are explicitly linking to the data.json file to generate an example event.

I think the only two requirements here are:

  1. We can generate sample events for the metricsets in each module.
  2. We do not remove or break the example events that are already in our documentation.

We can change the name of the .json file and the number of .json files generated as long as we update the links in the documentation to account for that.

We want to always create just one data.json file and never have the possibility to create more than one for different configs, this PR is breaking that

I don't think only having one data.json file is a requirement. We can link to multiple different files as needed.

If we want to fix it, this PR is needed for that, but that requires changing the path for the data.json files in some of the modules. If this is not done, the referenced data.json files in the docs will stop being updated

We can change the path to the generated files as needed, we just need to also update the relative paths referenced in the .asciidoc files that are used to generate the documentation as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can see that we are explicitly linking to the data.json file to generate an example event.

Yes, I noticed. To avoid changing the path then (I think this would be very simple, we would just need to change this part of the template), I set a default one specifically for the data.json file. By default this path is always the same, so if there are two tests overwritting this file (like it is happening with the module openmetrics), at least there is a chance now to change the path for one of the files if it is needed.

So this way we keep updating the old data.json files and give an option if we want to create more than one.

"@timestamp": "2019-03-01T08:05:34.853Z",
"event": {
"dataset": "openmetrics.collector",
"duration": 115000,
"module": "openmetrics"
},
"metricset": {
"name": "collector",
"period": 10000
},
"openmetrics": {
"labels": {
"job": "openmetrics"
},
"metrics": {
"up": 1
}
},
"service": {
"address": "127.0.0.1:55555",
"type": "openmetrics"
}
}
6 changes: 3 additions & 3 deletions metricbeat/module/openmetrics/collector/_meta/samelabeltestdata/docs.plain-expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
},
"openmetrics": {
"labels": {
"instance": "127.0.0.1:55922",
"instance": "127.0.0.1:42911",
"job": "openmetrics"
},
"metrics": {
Expand All @@ -35,7 +35,7 @@
},
"openmetrics": {
"labels": {
"instance": "127.0.0.1:55922",
"instance": "127.0.0.1:42911",
"job": "openmetrics",
"listener_name": "http"
},
Expand All @@ -49,4 +49,4 @@
"type": "openmetrics"
}
}
]
]
25 changes: 25 additions & 0 deletions metricbeat/module/openmetrics/collector/_meta/testdata/data.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
{
"@timestamp": "2019-03-01T08:05:34.853Z",
"event": {
"dataset": "openmetrics.collector",
"duration": 115000,
"module": "openmetrics"
},
"metricset": {
"name": "collector",
"period": 10000
},
"openmetrics": {
"labels": {
"job": "openmetrics"
},
"metrics": {
"up": 1
},
"type": "gauge"
},
"service": {
"address": "127.0.0.1:55555",
"type": "openmetrics"
}
}
26 changes: 13 additions & 13 deletions metricbeat/module/openmetrics/collector/_meta/testdata/docs.plain-expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,16 @@
"period": 10000
},
"openmetrics": {
"help": "Total number of connections opened to the listener of a given name.",
"labels": {
"instance": "127.0.0.1:55922",
"job": "openmetrics"
"instance": "127.0.0.1:37409",
"job": "openmetrics",
"listener_name": "http"
},
"metrics": {
"up": 1
"net_conntrack_listener_conn_accepted_total": 3
},
"type":"gauge"
"type": "counter"
},
"service": {
"address": "127.0.0.1:55555",
Expand All @@ -35,16 +37,14 @@
"period": 10000
},
"openmetrics": {
"help": "Total number of connections opened to the listener of a given name.",
"labels": {
"instance": "127.0.0.1:55922",
"job": "openmetrics",
"listener_name": "http"
"instance": "127.0.0.1:37409",
"job": "openmetrics"
},
"metrics": {
"net_conntrack_listener_conn_accepted_total": 3
"up": 1
},
"type":"counter"
"type": "gauge"
},
"service": {
"address": "127.0.0.1:55555",
Expand All @@ -64,18 +64,18 @@
"openmetrics": {
"help": "Total number of connections closed that were made to the listener of a given name.",
"labels": {
"instance": "127.0.0.1:55922",
"instance": "127.0.0.1:37409",
"job": "openmetrics",
"listener_name": "http"
},
"metrics": {
"net_conntrack_listener_conn_closed_total": 0
},
"type":"counter"
"type": "counter"
},
"service": {
"address": "127.0.0.1:55555",
"type": "openmetrics"
}
}
]
]
Loading