Skip to content
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

[receiver/windowseventlog] Add Windows Event Log Receiver #9228

Merged
merged 29 commits into from
May 26, 2022

Conversation

armstrmi
Copy link
Contributor

@armstrmi armstrmi commented Apr 12, 2022

Description:

Added Windows Event Logging Functionality based on the corresponding log-collection operator.

Link to tracking Issue:
(#9225)

Testing:
windowslog_test.go verifies that event logs can be read from the Windows API

Documentation:

(See README)

@armstrmi armstrmi requested review from a team and jpkrohling April 12, 2022 15:21
@jpkrohling
Copy link
Member

This is still pending a sponsor.

@armstrmi armstrmi requested a review from dmitryax as a code owner April 12, 2022 20:47
@djaglowski djaglowski changed the title Windows event log [receiver/windowslog] Add Windows Event Log Receiver Apr 14, 2022
@djaglowski djaglowski changed the title [receiver/windowslog] Add Windows Event Log Receiver [receiver/windowseventlog] Add Windows Event Log Receiver Apr 14, 2022
Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this receiver can only run on windows, please add a dummy implementation for non-windows systems, to ensure users get a clear error message.

go.mod Outdated Show resolved Hide resolved
receiver/windowslogreceiver/README.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
receiver/windowslogreceiver/README.md Outdated Show resolved Hide resolved
receiver/windowslogreceiver/README.md Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
@@ -0,0 +1,3 @@
2020-08-25 INFO Something routine
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this file used?

receiver/windowslogreceiver/windowslog.go Outdated Show resolved Hide resolved
receiver/windowslogreceiver/windowslog_test.go Outdated Show resolved Hide resolved
@armstrmi
Copy link
Contributor Author

Realize lots more files changed with this previous commit, will fix that momentarily

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good, but I've recently learned that windows unit tests are not currently running by default. For now, can you add a dedicated step to the build-and-test-windows.yml, like https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/9334/files#diff-5ca69371a0b431061a0b1b145a164ac5797c6a8bd745fba6061e3afe213318fd?

@armstrmi
Copy link
Contributor Author

Will be holding off on closing this PR due to the following reasons.

During integration tests, we noticed that certain fields of the Windows Event Log was not being populated by the receiver. Upon further inspection we notice that in our implemented code, to scrape some of the fields: message,level, opcode, etc. from the xml format, it would be under the header RenderingInfo. Unfortunately that RenderingInfo header only applies to events that have been collected by the Windows Event Collector, otherwise it simply does not exist in the XML of the log.

The underlying log-collection code that collects windows event logs should really collect from any service not just logs generated by the Windows Event Collector service. We have identified some changes we can make to the underlying code to make it more flexible.

We will be submitting an issue against the opentelemtry-log-collection repo detailing a proposed fix and work on the implementation.

This PR will be blocked by the above changes to the log-collection code.

@djaglowski
Copy link
Member

@armstrmi, #10187 switched all of the other log-collection operators over to using pkg/stanza instead. Will you please update this PR to do the same?

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's simplify the file organization. We should follow the typical naming convention for files differentiated by OS, which is to append _windows.go, _others.go, and correspondingly _windows_test.go and others_test.go.

I see that you've deduplicated some code by having windowslog.go for all OS's, but it's easier to understand the overall layout of the operator if these two sets of files are cleanly separated.

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from the file naming/organization changes and a couple nits, this looks good.

go.mod Outdated Show resolved Hide resolved
cmd/configschema/go.mod Outdated Show resolved Hide resolved
receiver/windowseventlogreceiver/windowslog_test.go Outdated Show resolved Hide resolved
receiver/windowseventlogreceiver/windowslog_test.go Outdated Show resolved Hide resolved
@djaglowski
Copy link
Member

@armstrmi, looks good except a check is failing:

go.mod/go.sum deps changes detected, please run "make gotidy" and commit the changes in this PR.
Error: Process completed with exit code 1.

Copy link
Member

@djaglowski djaglowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @armstrmi!

@djaglowski djaglowski merged commit cd59728 into open-telemetry:main May 26, 2022
@armstrmi armstrmi deleted the windows-event-log branch May 26, 2022 15:29
kentquirk pushed a commit to McSick/opentelemetry-collector-contrib that referenced this pull request Jun 14, 2022
…etry#9228)

* initial commit

added functionality for windows-log-event

* fixed go.sum files

* updated README

* added windows event log to receivers_test

* go mod tidy and added build flags

* updated package name

* dummy implementation created

* updated versions.yaml and codeowners

* updated logs received time

* updated changelog with windoweventlogreceiver

* cleaned up dependencies

* initial commit

added functionality for windows-log-event

* fixed go.sum files

* updated README

* added windows event log to receivers_test

* go mod tidy and added build flags

* updated package name

* updated xml_test to expect an array of interfaces

* updated wait time for receiving event logs

* fix winperfcounters

* updated go.mod to point to latest otel-log-collection

* removed otel log collection dependency

* go mod tidy

* added changes to go.sum and reordered file naming/organization

* ran make gotidy

* updated internal stanza version

* updated pkg/stanza for WEL

* added go.sum changes

* make gotidy
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants