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

[Beats][pytest] Asserting if filebeat logs include errors #20999

Merged
merged 6 commits into from
Oct 6, 2020
Merged

[Beats][pytest] Asserting if filebeat logs include errors #20999

merged 6 commits into from
Oct 6, 2020

Conversation

P1llus
Copy link
Member

@P1llus P1llus commented Sep 6, 2020 •
edited
Loading

What does this PR do?

First iteration on tackling this issue, allowing to assert on errors in filebeat, since any test will fail afterwards anyway, and the logs should not include errors. This could be anything from Elasticsearch not being available to pipeline failing to install.

Why is it important?

Ensure that a test is failing with the correct errors, currently if anything in the logs return an error, assertion will only include that it was a timeout, but it's actually an issue with for example pipeline installation.

  • 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 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.

Link related issues below. Insert the issue link or reference after the word "Closes" if merging this should automatically close it.

@P1llus P1llus requested a review from jsoriano September 6, 2020 19:22
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Sep 6, 2020
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Sep 6, 2020
@P1llus
Copy link
Member Author

P1llus commented Sep 6, 2020 •
edited
Loading

@jsoriano First iteration, and I think there is a few more things I need to fix.

However, I noticed that you are currently adding output directly to file, would it be okay for me to change stdout of the Popen to a PIPE, and then write it to a log? I need to use pipes to be able to read the stream live instead of reading the error afterwards.

Currently this will show if there is errors in the logs, but only after a timeout actually occurs, I was thinking maybe there was a reason for not using PIPE's, for example memory usage.

Also Popen does not close files afterwards, so you are actually opening new files constantly without closing them. A few different ways around it that I added in, but might change that afterwards.

@elasticmachine
Copy link
Collaborator

elasticmachine commented Sep 6, 2020 •
edited by jenkins-beats-ci bot
Loading

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Started by user Ivan Fernandez Calvo]

  • Start Time: 2020-10-06T07:35:20.403+0000

  • Duration: 50 min 37 sec

Test stats 🧪

Test Results
Failed 0
Passed 2470
Skipped 305
Total 2775

@andresrc andresrc added Team:Platforms Label for the Integrations - Platforms team Team:Services (Deprecated) Label for the former Integrations-Services team and removed Team: Ingest labels Sep 7, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations-platforms (Team:Platforms)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations-services (Team:Services)

jsoriano
jsoriano reviewed Sep 7, 2020
View reviewed changes
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 for working on this. I think this is going to be a nice improvement 🙂

I have added some suggestions.

filebeat/tests/system/test_modules.py Outdated Show resolved Hide resolved
filebeat/tests/system/test_modules.py Outdated Show resolved Hide resolved
filebeat/tests/system/test_modules.py Show resolved Hide resolved
filebeat/tests/system/test_modules.py Outdated Show resolved Hide resolved
jsoriano
jsoriano approved these changes Oct 1, 2020
View reviewed changes
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! Please backport this to 7.x.

filebeat/tests/system/test_modules.py Show resolved Hide resolved
@jsoriano jsoriano added needs_backport PR is waiting to be backported to other branches. v7.10.0 labels Oct 1, 2020
@P1llus
Copy link
Member Author

P1llus commented Oct 6, 2020

jenkins test this please

@P1llus P1llus merged commit 4dd8061 into elastic:master Oct 6, 2020
marc-gr pushed a commit to marc-gr/beats that referenced this pull request Oct 6, 2020
@P1llus @marc-gr
[Beats][pytest] Asserting if filebeat logs include errors (elastic#20999
0422263 
)

First iteration on tackling this issue, allowing to assert on errors in filebeat, since any test will fail afterwards anyway, and the logs should not include errors. This could be anything from Elasticsearch not being available to pipeline failing to install.

(cherry picked from commit 4dd8061)
@marc-gr marc-gr mentioned this pull request Oct 6, 2020
Merged
5 tasks
@marc-gr marc-gr removed the needs_backport PR is waiting to be backported to other branches. label Oct 6, 2020
marc-gr added a commit that referenced this pull request Oct 6, 2020
@marc-gr @P1llus
[Beats][pytest] Asserting if filebeat logs include errors (#20999) (#…
bad38ab 
…21554)

First iteration on tackling this issue, allowing to assert on errors in filebeat, since any test will fail afterwards anyway, and the logs should not include errors. This could be anything from Elasticsearch not being available to pipeline failing to install.

(cherry picked from commit 4dd8061)

Co-authored-by: Marius Iversen <[email protected]>
v1v added a commit to v1v/beats that referenced this pull request Oct 6, 2020
@v1v
Merge remote-tracking branch 'upstream/master' into feature/windows-2016
fcd6db3 
* upstream/master:
  [CI] Setup git config globally (elastic#21562)
  docs: update generate_fields_docs.py (elastic#21359)
  Add support for additional fields from V2 ALB logs (elastic#21540)
  Move Prometheus query & remote_write to GA (elastic#21507)
  feat: add a new step to run the e2e tests for certain parts of Beats (elastic#21100)
  [Elastic Agent] Add elastic agent ID and version to events from filebeat and metricbeat. (elastic#21543)
  Release cloudfoundry input and processor as GA (elastic#21525)
  [Packetbeat] New SIP protocol (elastic#21221)
  [Filebeat][New Module] Add support for Microsoft MTP / 365 Defender (elastic#21446)
  [Beats][pytest] Asserting if filebeat logs include errors (elastic#20999)
  junipersrx-module initial release (elastic#20017)
  Add a persistent cache for cloudfoundry metadata based on badger (elastic#20775)
  Add missing changelog entry for cisco umbrella (elastic#21550)
  [Elastic Agent] Add upgrade CLI to initiate upgrade of Agent locally (elastic#21425)
  Enable filestream input (elastic#21533)
  Add filestream input reader (elastic#21481)
  [CI] fix 'no matches found within 10000' (elastic#21466)
  Fix billing.go aws.GetStartTimeEndTime (elastic#21531)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsoriano jsoriano jsoriano approved these changes

Assignees
No one assigned
Labels
enhancement Team:Platforms Label for the Integrations - Platforms team Team:Services (Deprecated) Label for the former Integrations-Services team v7.10.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ensuring pytest catches pipeline errors
5 participants
@P1llus @elasticmachine @jsoriano @andresrc @marc-gr