[health-report CI] Print Logstash logs when pipeline faces an issue. #17991
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
|
This pull request does not have a backport label. Could you fix it @mashhurs? 🙏
|
mashhurs
changed the title
| # it is helpful when testing crash pipeline cases | ||
| config_path = os.getcwd() + "/.buildkite/scripts/health-report-tests/config" | ||
| process = subprocess.Popen(["bin/logstash", "--config.reload.automatic", "--path.settings", config_path, | ||
| "-w 1"], stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True, shell=False) |
There was a problem hiding this comment.
review note: in pipeline.yaml config files (e.g here), we set worker count, this is not necessary.
There was a problem hiding this comment.
Confirmed, this flag is not necessary and arguably was not appropriate for a generalized run_logstash method anyway. I like removal 👍
| full_error_message = (error_message + ", output: " + result.stdout) \ | ||
| if result.stdout else error_message | ||
| raise Exception(f"{full_error_message}") | ||
| print(result.stdout) |
There was a problem hiding this comment.
review note: regardless of failure status, prints log for better visibility.
There was a problem hiding this comment.
Nice. Looks like the majority of the logging that is beneficial will happen at https://github.com/elastic/logstash/pull/17991/files/acc787b0aeac14acc37704857dccddd75661990f#diff-cabfb7b7a18cf3ea856fc69cec74a74345761db2625d41b10ceb7ec13c447252R85 but this is good too i think.
|
💚 Build Succeeded
History
|
|
Love the extra logging. That makes this much easier to reason through! I think this is pointing to #17992. I believe the |
| return release_version | ||
|
|
||
| def install_plugin(self, plugin_path: str) -> None: | ||
| print("Installing logstash-integration-failure_injector plugin") |
There was a problem hiding this comment.
Is this a generalized method? Does this print statement belong here specific to one plugin?
There was a problem hiding this comment.
specific to one plugin?
Yup! The logstash-integration-failure_injector isn't embedded in the core by default and we install while health-report integration tests to simulate various scenarios (failure, backpressure, etc...). This log statement is very useful because a) plugin-manager (logstash-plugin {operation}) silently processes its operations. If any failure we get while installing the plugin w/o this log statement that b) might be confusing if it came from LS build.
There was a problem hiding this comment.
These improvements make it much easier to reason about failures in this pipeline 🎉
Just one question about a message #17991 (comment)
3 participants
Release notes
[rn:skip]
What does this PR do?
/_health_reportisn't reachable;Util#run_or_raise_error) regardless of the LS status for better visibilityWhy is it important/What is the impact to the user?
N.A
Checklist
[ ] My code follows the style guidelines of this project[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files (and/or docker env variables)[ ] I have added tests that prove my fix is effective or that my feature worksAuthor's Checklist
How to test this PR locally
Related issues
Use cases
Screenshots
Logs