-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Make beats properly exit #736
Conversation
c912e2a
to
d610f04
Compare
err := b.Start() | ||
|
||
if err != nil { | ||
// TODO: detect if logging was already fully setup or not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe that should be logic that gets embedded into logp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would actually be nice. So we could use logp.* in all cases and it would decide if it needs fmt.* or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened an issue here to solve this later: #746
LGTM other than a few minor suggestions. |
@andrewkroh Inputs fixed and rebased again. |
assert True == self.log_contains("Failed to read") | ||
|
||
|
||
def test_invalid_config(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest also a test for the -testconfig
option with a valid configuration file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
// logging not yet initialized, so using fmt.Printf | ||
fmt.Printf("Loading config file error: %v\n", err) | ||
os.Exit(1) | ||
return fmt.Errorf("Loading config file error: %v\n", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by convention error strings should start with lowercase, as these are often embedded in some other message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Looks like we don't follow this rule so far in beats.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made all error messages lower case as discussed.
Parts of libbeat were exiting the application directly by using os.Exit(). This makes it impossible always clean up the application before cleanup. In addition, a manager which is part of a beat gets also exited in case the beat exits, which is not inteded. * os.Exit() was removed from all places * Additional tests were added to test exit behaviour * Test for version check was added * Add test for -configtest flag * Make error log messages starting lowercase
Parts of libbeat were exiting the application directly by using os.Exit(). This makes it impossible always clean up the application before cleanup. In addition, a manager which is part of a beat gets also exited in case the beat exits, which is not inteded.