Use dedicated zapcore.Core for Windows service #3147
Conversation
rmfitzpatrick
force-pushed
the
windows_zapcore
branch
from
e2abd7b
to
546e588
Compare
There was a problem hiding this comment.
I thought I commented on this PR, apparently I forgot to submit and lost the comment, so doing it again.
I did not know we send our logs to Event Viewer on Windows.
Do we consider this to be a best practice? I think vast majority of app I saw on Windows simply log to a text file. Are we sure we are not doing a weird thing here by logging to Event Viewer?
(I understand it may be an official best practice recommended by Microsoft, but the tradition usually trumps official and I have yet to see any Windows app not written by Microsoft that puts its own logs in the Windows Event System).
Thoughts?
(Sorry for bringing this up only now, but I would like to know everyone's opinion and maybe delete the whole implementation if we don't believe it is the right approach).
|
@open-telemetry/collector-approvers (especially if you have Windows experience) what do you think about my comment above? |
|
@reyang @victlu we need help :) maybe someone from MS can help us with this. See #3147 (review) for the questions |
|
/cc @james-bebbington for thoughts about the initial implementation |
|
My 2c: event viewer for logging brings more pain than benefits. If there any chance this can be abstracted out into |
|
@tigrannajaryan @bogdandrutu |
Does these platforms justify this dependency on event log? Also, I don't think there were any efforts/recommendations writing to event log from Java apps, for example when I read https://docs.microsoft.com/en-us/azure/app-service/troubleshoot-diagnostic-logs#supported-log-types |
|
Not trying to weigh in on the windows service option's implementation and #2756 being under the radar, I will say the troubleshooting experience of the collector as a windows service is suboptimal and these changes provide much needed context for component issues. They don't resolve the problem for cases where other components stand up their own logger (like logging exporter) unless a shared core/sink is provided. |
@victlu do I understand it correct that if you run the Collector under Azure outputting the logs to Windows Event log results in more accessible logs? Is there a UI or something that shows them but no easy way to just see a file on the machine? Can you expand a bit? If this is the case we may want to support it, but perhaps it can be a command line option and by default we output to stdout as usual. |
|
Everyone, I created a separate issue to discuss whether we want to output to Event Viewer or no here #3241 Please continue the discussion there. I am going to review/merge this bug fix for now since it appears to be an obvious bug, but I still would like to discuss and decide whether going forward we want to keep the current behavior or delete and resort to regular logs. |
rmfitzpatrick
force-pushed
the
windows_zapcore
branch
from
546e588
to
6546214
Compare
|
@rmfitzpatrick does this fully fix #2756 ? |
I believe so in general, but if there are still outliers (like not having events for standalone loggers) new issues could be reported w/ more context whose solutions would be guided by #3241's outcomes. |
As reported in open-telemetry#2756 Windows service events only contain the zap entry message and none of the fields. These changes create a dedicated `zapcore.Core` to wrap the service logger's Core and use a production console encoder to encode the full entry, including its fields and stack if provided. **Link to tracking Issue:** open-telemetry#2756
Description:
Fixing a bug - As reported in #2756 Windows service events only contain the zap entry message and none of the fields. These changes create a dedicated
zapcore.Coreto wrap the service logger's Core and use a production console encoder to encode the full entry, including its fields and stack if provided.Link to tracking Issue:
#2756
Testing:
Tested manually with windows service.
Currently examining unit test in addition to existing service test.edit: I don't believe there's a clean* way to mock
eventlog.Logsending in a unit test since* no interfaces are used and it resolves to a syscall: https://github.com/golang/sys/blob/94ec62e081691fed0145b707594111cee9cb360a/windows/zsyscall_windows.go#L1035.