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

[pkg/stanza] fix eventData format for Windows events #20548

Conversation

swiatekm
Copy link
Contributor

@swiatekm swiatekm commented Apr 3, 2023

Description:
Fixing #20547. I'm parsing the eventData field of Windows events into a slice of structs, and then converting it to either a map or a slice of strings, depending on if they have names.

Link to tracking Issue: #20547.

Testing:
Modified existing test data to use the new format, and added new tests for the conversion.

@swiatekm swiatekm requested a review from a team April 3, 2023 16:22
@runforesight
Copy link

runforesight bot commented Apr 3, 2023

Foresight Summary

    
Major Impacts

build-and-test-windows duration(8 seconds) has decreased 30 minutes 36 seconds compared to main branch avg(30 minutes 44 seconds).
View More Details

⭕  build-and-test-windows workflow has finished in 8 seconds (30 minutes 36 seconds less than main branch avg.) and finished at 10th Apr, 2023.


Job Failed Steps Tests
windows-unittest-matrix -     🔗  N/A See Details
windows-unittest -     🔗  N/A See Details

✅  check-links workflow has finished in 47 seconds and finished at 10th Apr, 2023.


Job Failed Steps Tests
changed files -     🔗  N/A See Details
check-links -     🔗  N/A See Details

✅  telemetrygen workflow has finished in 1 minute 27 seconds and finished at 10th Apr, 2023.


Job Failed Steps Tests
build-dev -     🔗  N/A See Details
publish-latest -     🔗  N/A See Details
publish-stable -     🔗  N/A See Details

✅  changelog workflow has finished in 2 minutes 16 seconds and finished at 10th Apr, 2023.


Job Failed Steps Tests
changelog -     🔗  N/A See Details

✅  prometheus-compliance-tests workflow has finished in 3 minutes (3 minutes 19 seconds less than main branch avg.) and finished at 10th Apr, 2023.


Job Failed Steps Tests
prometheus-compliance-tests -     🔗  N/A See Details

✅  load-tests workflow has finished in 6 minutes 18 seconds (4 minutes 1 second less than main branch avg.) and finished at 10th Apr, 2023.


Job Failed Steps Tests
setup-environment -     🔗  N/A See Details
loadtest (TestIdleMode) -     🔗  N/A See Details
loadtest (TestBallastMemory|TestLog10kDPS) -     🔗  N/A See Details
loadtest (TestMetric10kDPS|TestMetricsFromFile) -     🔗  N/A See Details
loadtest (TestMetricResourceProcessor|TestTrace10kSPS) -     🔗  N/A See Details
loadtest (TestTraceNoBackend10kSPS|TestTrace1kSPSWithAttrs) -     🔗  N/A See Details
loadtest (TestTraceBallast1kSPSWithAttrs|TestTraceBallast1kSPSAddAttrs) -     🔗  N/A See Details
loadtest (TestTraceAttributesProcessor) -     🔗  N/A See Details

✅  e2e-tests workflow has finished in 11 minutes 8 seconds (2 minutes 57 seconds less than main branch avg.) and finished at 10th Apr, 2023.


Job Failed Steps Tests
kubernetes-test (v1.26.0) -     🔗  N/A See Details
kubernetes-test (v1.25.3) -     🔗  N/A See Details
kubernetes-test (v1.24.7) -     🔗  N/A See Details
kubernetes-test (v1.23.13) -     🔗  N/A See Details

✅  build-and-test workflow has finished in 36 minutes 32 seconds (10 minutes 4 seconds less than main branch avg.) and finished at 10th Apr, 2023.


Job Failed Steps Tests
setup-environment -     🔗  N/A See Details
check-codeowners -     🔗  N/A See Details
check-collector-module-version -     🔗  N/A See Details
build-examples -     🔗  N/A See Details
checks -     🔗  N/A See Details
correctness-metrics -     🔗  N/A See Details
correctness-traces -     🔗  N/A See Details
integration-tests -     🔗  N/A See Details
lint-matrix (receiver-0) -     🔗  N/A See Details
lint-matrix (receiver-1) -     🔗  N/A See Details
lint-matrix (processor) -     🔗  N/A See Details
lint-matrix (exporter) -     🔗  N/A See Details
lint-matrix (extension) -     🔗  N/A See Details
lint-matrix (connector) -     🔗  N/A See Details
lint-matrix (internal) -     🔗  N/A See Details
lint-matrix (other) -     🔗  N/A See Details
unittest-matrix (1.20, receiver-0) -     🔗  N/A See Details
unittest-matrix (1.20, receiver-1) -     🔗  N/A See Details
unittest-matrix (1.20, processor) -     🔗  N/A See Details
unittest-matrix (1.20, exporter) -     🔗  N/A See Details
unittest-matrix (1.20, extension) -     🔗  N/A See Details
unittest-matrix (1.20, connector) -     🔗  N/A See Details
unittest-matrix (1.20, internal) -     🔗  N/A See Details
unittest-matrix (1.20, other) -     🔗  N/A See Details
unittest-matrix (1.19, receiver-0) -     🔗  N/A See Details
unittest-matrix (1.19, receiver-1) -     🔗  N/A See Details
unittest-matrix (1.19, processor) -     🔗  N/A See Details
unittest-matrix (1.19, exporter) -     🔗  N/A See Details
unittest-matrix (1.19, extension) -     🔗  N/A See Details
unittest-matrix (1.19, connector) -     🔗  N/A See Details
unittest-matrix (1.19, internal) -     🔗  N/A See Details
unittest-matrix (1.19, other) -     🔗  N/A See Details
lint -     🔗  N/A See Details
unittest (1.20) -     🔗  N/A See Details
unittest (1.19) -     🔗  N/A See Details
cross-compile (darwin, amd64) -     🔗  N/A See Details
cross-compile (darwin, arm64) -     🔗  N/A See Details
cross-compile (linux, 386) -     🔗  N/A See Details
cross-compile (linux, amd64) -     🔗  N/A See Details
cross-compile (linux, arm) -     🔗  N/A See Details
cross-compile (linux, arm64) -     🔗  N/A See Details
cross-compile (linux, ppc64le) -     🔗  N/A See Details
cross-compile (windows, 386) -     🔗  N/A See Details
cross-compile (windows, amd64) -     🔗  N/A See Details
build-package (deb) -     🔗  N/A See Details
build-package (rpm) -     🔗  N/A See Details
windows-msi -     🔗  N/A See Details
publish-check -     🔗  N/A See Details
publish-stable -     🔗  N/A See Details
publish-dev -     🔗  N/A See Details
rotate-milestone -     🔗  N/A See Details

🔎 See details on Foresight

*You can configure Foresight comments in your organization settings page.

@djaglowski
Copy link
Member

Do you know if it's possible that the Name attribute could be present for some fields but not others?

It seems strange to me that we would return an unpredictable structure. What do you think about returning this as a slice of maps? (e.g. [ {name: "foo", value: "bar"}, {value: "world"} ] This would produce a consistent structure and also also could handle mixed use of the Name attribute.

@swiatekm
Copy link
Contributor Author

swiatekm commented Apr 4, 2023

Do you know if it's possible that the Name attribute could be present for some fields but not others?

I honestly don't know, not really an expert on Windows events. I'll see if I can dig something definitive out of Microsoft's documentation. My current understanding is that either all the fields have names, or none - I wrote the converter the way I did so the behaviour was defined in case I was wrong.

It seems strange to me that we would return an unpredictable structure. What do you think about returning this as a slice of maps? (e.g. [ {name: "foo", value: "bar"}, {value: "world"} ] This would produce a consistent structure and also also could handle mixed use of the Name attribute.

It'd be consistent, but I'm not sure if stanza or ottl are powerful enough to transform them back, and I do think these representations (map for named fields and slice for anonymous) are the most natural and convenient.

@swiatekm
Copy link
Contributor Author

swiatekm commented Apr 4, 2023

Technically, the Name attribute is optional: https://learn.microsoft.com/en-us/windows/win32/wes/eventschema-datafieldtype-complextype.

However, I checked what other agents do, and they basically all assume this always exists. See:

So maybe it's ok for this to always be a map?

@swiatekm
Copy link
Contributor Author

swiatekm commented Apr 6, 2023

@djaglowski how about just always returning a map and dropping entries without a Name? I'm fairly confident it always exists, and this is how every other agent I checked behaves.

@djaglowski
Copy link
Member

@swiatekm-sumo, that seems reasonable to me.

@swiatekm swiatekm force-pushed the feat/stanza/windowsinput/eventdata-map branch from 9aa8ea1 to e7f1ed7 Compare April 10, 2023 16:46
@swiatekm swiatekm force-pushed the feat/stanza/windowsinput/eventdata-map branch from e7f1ed7 to 4ad53ca Compare April 10, 2023 16:47
@swiatekm
Copy link
Contributor Author

@djaglowski done

@djaglowski djaglowski merged commit 74d2a14 into open-telemetry:main Apr 11, 2023
@github-actions github-actions bot added this to the next release milestone Apr 11, 2023
@swiatekm swiatekm deleted the feat/stanza/windowsinput/eventdata-map branch April 11, 2023 15:13
codeboten added a commit that referenced this pull request Apr 11, 2023
codeboten pushed a commit that referenced this pull request Apr 11, 2023
swiatekm pushed a commit to swiatekm/opentelemetry-collector-contrib that referenced this pull request Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants