-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
otelcol: fix windows event log core placement #11051
Conversation
In the CollectorSettings LoggingOptions, when the options are `zap.WrapCore`, the final core wrapped will end up running first. For this reason, the windowsEventLogCore must be the first option in the list of user supplied options, to ensure user supplied options are run before the core that writes to the Windows Event Log.
@@ -141,6 +138,19 @@ func openEventLog(serviceName string) (*eventlog.Log, error) { | |||
return elog, nil | |||
} | |||
|
|||
func loggingOptionsWithEventLogCore( |
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 extracted this to a function for 2 reasons:
- I was trying to write a unit test for it (failed to find a way).
- I wanted the comment explaining why the prepend is necessary to really stand out for any future contributors looking at this.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #11051 +/- ##
=======================================
Coverage 92.23% 92.23%
=======================================
Files 414 414
Lines 19810 19810
=======================================
Hits 18271 18271
Misses 1167 1167
Partials 372 372 ☔ View full report in Codecov by Sentry. |
I don't yet understand why this breaks the |
EDIT 2: Wow, so it was the double pointer argument all along. Insane that I didn't see that. 😡 |
I should have passed the service config as a double pointer, not as a pointer that I further reference within the function.
Thanks for spotting this @braydonk
So there were some options in telemetry, e.g.: log level, being ignored when logging to the event log? |
otelcol/collector_test.go
Outdated
@@ -564,6 +564,8 @@ func newEnvProvider() confmap.ProviderFactory { | |||
return confmap.NewRetrieved(float64(6.4)) | |||
case "env:BOOL": | |||
return confmap.NewRetrieved(true) | |||
case "env:ProgramData": |
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 is this needed? Was it added for the manual tests?
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.
Left in by accident from manual tests. Removed in d8e89c2
otelcol/collector_windows.go
Outdated
userOptions []zap.Option, | ||
) []zap.Option { | ||
return append( | ||
// The event log core must run last, so it needs to be the first option |
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.
Given that I already changed that order in the past (and can't recall if by accident or intentionally) explicit wording seems advisable:
// The event log core must run last, so it needs to be the first option | |
// The order below must be preserved - see PR #11051 | |
// The event log core must run last, so it needs to be the first option |
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.
Made the comment more explicit in cf2b4c5
# Include 'user' if the change is relevant to end users. | ||
# Include 'api' if there is a change to a library API. | ||
# Default: '[user]' | ||
change_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.
Isn't the result of the change visible to user
?
This specifically affects users who actually use the otelcol package to create their own Collector service. In my use case, I added LoggingOptions to change certain Collector self-logs to DEBUG level (effectively muting them in our case). When the order of the event log core was changed in the LoggingOptions, it meant that it always ran before these options we provided.
Not for users of an OpenTelemetry-project-provided Collector as far as I know. This change is only noticeable for people in my very specific case who provide LoggingOptions in their custom-built collector that runs on Windows as a service. 😄 |
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
Description
In the CollectorSettings LoggingOptions, when the options are
zap.WrapCore
, the final core wrapped will end up running first. For this reason, the windowsEventLogCore must be the first option in the list of user supplied options, to ensure user supplied options are run before the core that writes to the Windows Event Log.Prior art: I fixed this before in #5298.
Link to tracking issue
#5297
Testing
Testing was manual by running the Collector as a service after this change. I tried to add a unit test but could not find an effective way to do that due to all the type indirection done by the
zap
package. I was hoping to find an easy way to tell that thewindowsEventLogCore
is the first core wrapped in the options but I could not find a way to pull that off. Open to ideas if anyone can find a way.