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

Filebeat pytest improvements #30103

Merged

Conversation

matschaffer
Copy link
Contributor

@matschaffer matschaffer commented Jan 31, 2022

What does this PR do?

Provides a few improvements to the filebeat ingest pytests.

  • Include which file failed the "wait for docs" step
  • Support TESTING_FILEBEAT_FILEPATTERN to ingest a single file
  • Include the full event in the "not documented" case

Why is it important?

Makes it easier to write and maintain filebeat pytests

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (I'll put these in Updated dev guide instructions to work with master #30014)
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

# Start an ES, give it an admin/changeme user)

# setup the test env
❯ cd filebeat
❯ make update filebeat.test
❯ make python-env
❯ source ./build/python-env/bin/activate

# Run a pattered test
❯ INTEGRATION_TESTS=1 BEAT_STRICT_PERMS=false TESTING_FILEBEAT_MODULES=elasticsearch TESTING_FILEBEAT_FILESETS=slowlog TESTING_FILEBEAT_FILEPATTERN=es_indexing_slowlog.800.log ES_PASS=changeme pytest tests/system/test_modules.py -v --full-trace

# Add a few field 
❯ echo '{"ecs.version": "1.2", "newfield": "matwazhere", "elasticsearch.slowlog.message": "need a message to grok"}' >> module/elasticsearch/slowlog/test/es_indexing_slowlog.800.log
❯ INTEGRATION_TESTS=1 BEAT_STRICT_PERMS=false TESTING_FILEBEAT_MODULES=elasticsearch TESTING_FILEBEAT_FILESETS=slowlog TESTING_FILEBEAT_FILEPATTERN=es_indexing_slowlog.800.log ES_PASS=changeme pytest tests/system/test_modules.py -v --full-trace
...
E               Exception: Key 'newfield' found in event ({'agent': {'name': 'matschaffer-mbp2019.lan', 'id': '4cda5c97-4168-4090-911e-3c34c1ae00aa', 'type': 'filebeat', 'ephemeral_id': '158a6d12-a3cc-4794-b14c-0c0cd424ad57', 'version': '8.1.0'}, 'log': {'file': {'path': '/Users/matschaffer/elastic/beats/filebeat/module/elasticsearch/slowlog/test/es_indexing_slowlog.800.log'}, 'offset': 1488}, 'fileset': {'name': 'slowlog'}, 'message': 'need a message to grok', 'newfield': 'matwazhere', 'input': {'type': 'log'}, '@timestamp': '2022-01-31T05:09:58.703Z', 'ecs': {'version': '1.2'}, 'elasticsearch': {'slowlog': {}}, 'service': {'type': 'elasticsearch'}, 'host': {'name': 'matschaffer-mbp2019.lan'}, 'event': {'ingested': '2022-01-31T05:09:59.729047Z', 'created': '2022-01-31T05:09:58.703Z', 'kind': 'event', 'module': 'elasticsearch', 'category': 'database', 'dataset': 'elasticsearch.slowlog'}}) is not documented!

# Kill all the docs
❯ echo '{"gonna get dropped": true}' > module/elasticsearch/slowlog/test/es_indexing_slowlog.800.log
❯ INTEGRATION_TESTS=1 BEAT_STRICT_PERMS=false TESTING_FILEBEAT_MODULES=elasticsearch TESTING_FILEBEAT_FILESETS=slowlog TESTING_FILEBEAT_FILEPATTERN=es_indexing_slowlog.800.log ES_PASS=changeme pytest tests/system/test_modules.py -v --full-trace
...
E               beat.beat.TimeoutError: Timeout waiting for 'indices present for /Users/matschaffer/elastic/beats/filebeat/tests/system/../../module/elasticsearch/slowlog/test/es_indexing_slowlog.800.log' to be true. Waited 10 seconds.

# Undo the changes
❯ git checkout module/elasticsearch/slowlog/test/es_indexing_slowlog.800.log

Related issues

I ended up writing these as part of #30018 but didn't want to mix them into that PR.

This surfaces which file the test was waiting to see documents from. Otherwise you just get a numeric test failure with no clear idea of which file isn't ingesting as expected.
For example:

```
INTEGRATION_TESTS=1 BEAT_STRICT_PERMS=false TESTING_FILEBEAT_MODULES=elasticsearch TESTING_FILEBEAT_FILESETS=slowlog TESTING_FILEBEAT_FILEPATTERN=es_indexing_slowlog.800.log ES_PASS=changeme pytest tests/system/test_modules.py -v --full-trace
```
By seeing the event, we can more easily identify which log file it's coming from.
@matschaffer matschaffer self-assigned this Jan 31, 2022
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jan 31, 2022
@matschaffer matschaffer added Filebeat Filebeat Team:Integrations Label for the Integrations team labels Jan 31, 2022
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations (Team:Integrations)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jan 31, 2022
@matschaffer matschaffer added the backport-v8.0.0 Automated backport with mergify label Jan 31, 2022
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jan 31, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Reason: null

  • Start Time: 2022-01-31T16:05:55.029+0000

  • Duration: 128 min 26 sec

  • Commit: 94130de

Test stats 🧪

Test Results
Failed 0
Passed 48003
Skipped 4292
Total 52295

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@ruflin
Copy link
Member

ruflin commented Jan 31, 2022

Mentioning @jsoriano and @mtojek here to be aware of the changes to the system tests of Filebeat. My assumption is that any improvements we make to scripts are also helpful in the case of package development.

@@ -107,7 +107,7 @@ def load_fileset_test_cases():
continue

test_files = glob.glob(os.path.join(modules_dir, module,
fileset, "test", "*.log"))
fileset, "test", os.getenv("TESTING_FILEBEAT_FILEPATTERN", "*.log")))
Copy link
Member

Choose a reason for hiding this comment

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

@ph @cmacknz General problem we have with these environment variables is that everyone that worked on the test suite knows about these, everyone else has to find them in the code base ... We should find a better place (not related to this PR).

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree

I think we should rethink how and why we introduce variables in the first place. The problem today is there are multiple ways to run tests that support or require different side effects.

I strongly think the test cli should give me all the required information for options without looking in the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely agreed.

Not sure if pytest offers something like this directly. mage pythonIntegTest has inline help, but does a lot of extra work (15min for my laptop).

I guess maybe one option would be a lighter mage target with these options covered in the inline help.

In working on the tests I also noticed some similarity with metricbeat testdata integration tests. For example:

func TestFetchEventContents(t *testing.T) {

If we did these filebeat module tests as regular golang test cases, stuff like TESTING_FILEBEAT_FILEPATTERN would just be go test -run (pattern)

Copy link
Contributor

Choose a reason for hiding this comment

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

Having quicker mage task is an option.

I would like to remove all python integration tests and use a single language.

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 feel that. 🧡

Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

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

Change LGTM

@matschaffer matschaffer merged commit 3ddf236 into elastic:master Feb 1, 2022
@matschaffer matschaffer deleted the filebeat-pytest-improvements branch February 1, 2022 00:11
mergify bot pushed a commit that referenced this pull request Feb 1, 2022
* Include the test_file as wait_until name
* Support TESTING_FILEBEAT_FILEPATTERN to test a single ingest file
* Include full event on "not documented" case

(cherry picked from commit 3ddf236)
matschaffer added a commit that referenced this pull request Feb 1, 2022
* Include the test_file as wait_until name
* Support TESTING_FILEBEAT_FILEPATTERN to test a single ingest file
* Include full event on "not documented" case

(cherry picked from commit 3ddf236)

Co-authored-by: Mat Schaffer <mat@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v8.0.0 Automated backport with mergify Filebeat Filebeat Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants