-
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
Add log output to Windows Event Log #5913
Add log output to Windows Event Log #5913
Conversation
5728357
to
7ab16d3
Compare
I rebased this to fix the CHANGELOG merge conflict. |
#logging.to_syslog: false | ||
|
||
# Send all logging output to Windows Event Logs. The default is false. | ||
#logging.to_eventlog: false |
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.
Not a big fan of naming we have for historical reason and I'm thinking we should change it for 7.0. Perhaps we could already follow here a new scheme similar to what we have in other places:
logging.eventlog.enabled
. Like this we can have specific logging options under the namespace.
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 am 100% on board with changing how logging is configured for 7.0. I did it this way to be consistent. I will open an issue to track things that I'd like to "break" for 7.0 (I have a list 😄 ).
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.
It should be possible to already do the "new" scheme here as so far it's just a name. More thinking then we have to break one option less for 7 :-) But not sure if we are already settle on if that should be new scheme.
+1 on the issue.
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.
Well the scheme I had in mind kind of conflicts. I was thinking of something like.
logging.output:
- type: file
level: info
json: true
- type: eventlog
level: warn
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.
got it, then lets keep it as is in your PR and break it later so we don't need to come to a conclusion now.
I like your format as it gives max flexibility. I assume you have in mind that more then 1 log output can be enabled?
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.
Yeah, more than one output and possibly more than one instance of an output type (not 100% sure yet; requires more thought).
@@ -6,6 +6,21 @@ import ( | |||
"go.uber.org/zap/zapcore" | |||
) | |||
|
|||
var baseEncodingConfig = zapcore.EncoderConfig{ |
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.
+1 on the cleanup
libbeat/logp/eventlog_windows.go
Outdated
) | ||
|
||
const ( | ||
eventID = 100 |
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.
Does 100
have a special meaning in windows event logs?
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.
No, it's completely arbitrary. The value needs to be between 1 and 1000. Most of the original log events have three digit IDs so I just chose 100.
libbeat/logp/eventlog_windows.go
Outdated
|
||
const ( | ||
eventID = 100 | ||
supports = eventlog.Warning | eventlog.Info | eventlog.Info |
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.
Why does eventlog.Info
show up twice? Intentional or should it be Debug?
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 should be Error
. Will push a fix.
@@ -53,7 +53,7 @@ func (c *syslogCore) Write(entry zapcore.Entry, fields []zapcore.Field) error { | |||
} | |||
|
|||
// Console encoder writes tabs which don't render nicely with syslog. | |||
replaceTabsWithSpaces(buffer.Bytes(), 2) | |||
replaceTabsWithSpaces(buffer.Bytes(), 4) |
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.
Is that change related to this PR?
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.
It's not related. I just realized that there can be up to 4 tabs.
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.
LGTM.
@andrewkroh Should I squash?
By setting `logging.to_eventlog: true` all log output will be written to the Application log. The source name will be name of the Beat.
ffcfc92
to
7bcfd12
Compare
I just did a manual squash to keep the vendor changes separate from the code. Now you can do a rebase and merge. Thanks |
By setting
logging.to_eventlog: true
all log output will be writtento the Application log. The source name will be name of the Beat.