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.

Sign up for GitHub

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

Jump to bottom

[windowseventlogreceiver] EventData is not extracted correctly #24493

Closed
sumo-drosiek opened this issue Jul 24, 2023 · 8 comments · Fixed by #28587
Closed

[windowseventlogreceiver] EventData is not extracted correctly #24493

sumo-drosiek opened this issue Jul 24, 2023 · 8 comments · Fixed by #28587
Labels
bug Something isn't working pkg/stanza

Comments

@sumo-drosiek
Copy link
Member

sumo-drosiek commented Jul 24, 2023 •
edited
Loading

Component(s)

pkg/stanza

What happened?

Description

EventData from the following xml is omitted:

- <Event xmlns="http://schemas.microsoft.com/win/2004/08/events/event">
- <System>
  <Provider Name="OtelcolSumo" /> 
  <EventID Qualifiers="0">1</EventID> 
  <Version>0</Version> 
  <Level>4</Level> 
  <Task>0</Task> 
  <Opcode>0</Opcode> 
  <Keywords>0x80000000000000</Keywords> 
  <TimeCreated SystemTime="2023-07-24T18:10:39.1283657Z" /> 
  <EventRecordID>8907</EventRecordID> 
  <Correlation /> 
  <Execution ProcessID="0" ThreadID="0" /> 
  <Channel>Application</Channel> 
  <Computer>DESKTOP-LN8GBII</Computer> 
  <Security /> 
  </System>
- <EventData>
  <Data>1.6902222391283658e+09 info ResourceLog #0 Resource SchemaURL: https://opentelemetry.io/schemas/1.6.1 Resource attributes: -> host.name: Str(DESKTOP-LN8GBII) -> os.type: Str(windows) -> host.id: Str(f30cad60-245d-491c-9f48-794f52460b42) -> sumo.datasource: Str(windows) -> _sourceCategory: Str(otel/windows) ScopeLogs #0 ScopeLogs SchemaURL: InstrumentationScope LogRecord #0 ObservedTimestamp: 2023-07-24 18:10:38.3322332 +0000 UTC Timestamp: 2023-07-24 18:10:38.1006007 +0000 UTC SeverityText: INFO SeverityNumber: Info(9) Body: Map({"channel":"Application","computer":"DESKTOP-LN8GBII","event_data":{},"event_id":{"id":1,"qualifiers":0},"keywords":["0x80000000000000"],"level":"4","message":"","opcode":"0","provider":{"event_source":"","guid":"","name":"OtelcolSumo"},"record_id":8903,"system_time":"2023-07-24T18:10:38.1006007Z","task":"0"}) Trace ID: Span ID: Flags: 0 LogRecord #1 ObservedTimestamp: 2023-07-24 18:10:38.3328803 +0000 UTC Timestamp: 2023-07-24 18:10:38.1040532 +0000 UTC SeverityText: INFO SeverityNumber: Info(9) Body: Map({"channel":"Application","computer":"DESKTOP-LN8GBII","event_data":{},"event_id":{"id":1,"qualifiers":0},"keywords":["0x80000000000000"],"level":"4","message":"","opcode":"0","provider":{"event_source":"","guid":"","name":"OtelcolSumo"},"record_id":8904,"system_time":"2023-07-24T18:10:38.1040532Z","task":"0"}) Trace ID: Span ID: Flags: 0 LogRecord #2 ObservedTimestamp: 2023-07-24 18:10:38.3347101 +0000 UTC Timestamp: 2023-07-24 18:10:38.3312793 +0000 UTC SeverityText: ERROR SeverityNumber: Error(17) Body: Map({"channel":"Application","computer":"DESKTOP-LN8GBII","event_data":{},"event_id":{"id":3,"qualifiers":0},"keywords":["0x80000000000000"],"level":"2","message":"","opcode":"0","provider":{"event_source":"","guid":"","name":"OtelcolSumo"},"record_id":8905,"system_time":"2023-07-24T18:10:38.3312793Z","task":"0"}) Trace ID: Span ID: Flags: 0 {"kind": "exporter", "data_type": "logs", "name": "logging"}</Data> 
  </EventData>
  </Event>

This is probably due to this part of code:

opentelemetry-collector-contrib/pkg/stanza/operator/input/windows/xml.go

Lines 145 to 147 in 13ff018

if entry.Name != "" {
outputMap[entry.Name] = entry.Value
}

Steps to Reproduce

I'm running Sumo Logic Distribution for OpenTelemetry Collector as Windows service

Expected Result

"event_data" containing data

Actual Result

"event_data":{}

Collector version

v0.81.0

Environment information

Environment

OS: (e.g., "Ubuntu 20.04")
Compiler(if manually compiled): (e.g., "go 14.2")

OpenTelemetry Collector configuration

No response

Log output

No response

Additional context

No response
@sumo-drosiek sumo-drosiek added bug Something isn't working needs triage New item requiring triage labels Jul 24, 2023
@github-actions
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@sumo-drosiek
Copy link
Member Author

cc: @swiatekm-sumo as he modified this part of code recently

@swiatekm
Copy link
Contributor

This was changed to the current state on the assumption that all built-in events have named Data entries in that fields, which appears to be true. I guess custom events can still have whatever they want in there. Originally, I wanted this to be an array in this case, but @djaglowski argued the type should always be the same. Maybe we should make this behaviour configurable?

@sumo-drosiek
Copy link
Member Author

Can we use empty entry.Name?

@swiatekm
Copy link
Contributor

Can we use empty entry.Name?

We can, but if there's more than one entry, we'll only keep the last one. In that case, I think we should have a different data structure here.

@djaglowski
Copy link
Member

Here is my original comment on this.

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.

Should we try the slice of maps approach?

@sumo-drosiek
Copy link
Member Author

Should we try the slice of maps approach?

I think it is reasonable. However it sounds like breaking change

This was referenced Sep 3, 2023
Open
Open
This was referenced Sep 6, 2023
Open
Open
This was referenced Sep 12, 2023
Open
Open
@github-actions
Copy link
Contributor

Pinging code owners for pkg/stanza: @djaglowski. See Adding Labels via Comments if you do not have permissions to add labels yourself.

@djaglowski djaglowski removed the needs triage New item requiring triage label Sep 13, 2023
@pjanotti pjanotti mentioned this issue Oct 24, 2023
Merged
djaglowski added a commit that referenced this issue Oct 27, 2023
@pjanotti @djaglowski
[pkg/stanza][receiver/windowseventlog] Improve EventDataType support …
5b36953 
…in EventXML (Windows) (#28587)

**Description:**
The [XML schema for Windows events supports `Data` elements without the
`Name`
attribute](https://learn.microsoft.com/en-us/windows/win32/wes/eventschema-datafieldtype-complextype),
however, the current implementation doesn't capture `Data` elements
without the `Name` attribute.

Capturing such elements is specially important for events for which the
publisher metadata is invalid. These elements contain the data that will
give a user a much better chance of actually understanding the event,
see
[here](#21491 (comment))
for an example.

I'm adding also the optional `Binary` element. Although this element
typically requires knowledge of the actual data type it is representing
sometimes it can be useful together with the data elements.

I consider this to be a breaking change because it modifies the layout
of the event generated by the package. It isn't an addition, the old
representation is changed, please refer to the changes in tests to see
the difference.

**Link to tracking Issue:**
This is the last pending item to fix #24493, #21491 ([item
5](#21491)).

**Testing:**
- Local run of the affected receiver and package
- "Run Windows" on my fork

**Documentation:**
N/A

---------

Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com>
jmsnll pushed a commit to jmsnll/opentelemetry-collector-contrib that referenced this issue Nov 12, 2023
@pjanotti @djaglowski @jmsnll
[pkg/stanza][receiver/windowseventlog] Improve EventDataType support …
992b1ea 
…in EventXML (Windows) (open-telemetry#28587)

**Description:**
The [XML schema for Windows events supports `Data` elements without the
`Name`
attribute](https://learn.microsoft.com/en-us/windows/win32/wes/eventschema-datafieldtype-complextype),
however, the current implementation doesn't capture `Data` elements
without the `Name` attribute.

Capturing such elements is specially important for events for which the
publisher metadata is invalid. These elements contain the data that will
give a user a much better chance of actually understanding the event,
see
[here](open-telemetry#21491 (comment))
for an example.

I'm adding also the optional `Binary` element. Although this element
typically requires knowledge of the actual data type it is representing
sometimes it can be useful together with the data elements.

I consider this to be a breaking change because it modifies the layout
of the event generated by the package. It isn't an addition, the old
representation is changed, please refer to the changes in tests to see
the difference.

**Link to tracking Issue:**
This is the last pending item to fix open-telemetry#24493, open-telemetry#21491 ([item
5](open-telemetry#21491)).

**Testing:**
- Local run of the affected receiver and package
- "Run Windows" on my fork

**Documentation:**
N/A

---------

Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com>
RoryCrispin pushed a commit to ClickHouse/opentelemetry-collector-contrib that referenced this issue Nov 24, 2023
@pjanotti @djaglowski @RoryCrispin
[pkg/stanza][receiver/windowseventlog] Improve EventDataType support …
196189e 
…in EventXML (Windows) (open-telemetry#28587)

**Description:**
The [XML schema for Windows events supports `Data` elements without the
`Name`
attribute](https://learn.microsoft.com/en-us/windows/win32/wes/eventschema-datafieldtype-complextype),
however, the current implementation doesn't capture `Data` elements
without the `Name` attribute.

Capturing such elements is specially important for events for which the
publisher metadata is invalid. These elements contain the data that will
give a user a much better chance of actually understanding the event,
see
[here](open-telemetry#21491 (comment))
for an example.

I'm adding also the optional `Binary` element. Although this element
typically requires knowledge of the actual data type it is representing
sometimes it can be useful together with the data elements.

I consider this to be a breaking change because it modifies the layout
of the event generated by the package. It isn't an addition, the old
representation is changed, please refer to the changes in tests to see
the difference.

**Link to tracking Issue:**
This is the last pending item to fix open-telemetry#24493, open-telemetry#21491 ([item
5](open-telemetry#21491)).

**Testing:**
- Local run of the affected receiver and package
- "Run Windows" on my fork

**Documentation:**
N/A

---------

Co-authored-by: Daniel Jaglowski <jaglows3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working pkg/stanza
Projects
None yet
Milestone
No milestone
4 participants
@djaglowski @sumo-drosiek @crobert-1 @swiatekm