-
Notifications
You must be signed in to change notification settings - Fork 129
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
Add LogRecordProcessor to record event log records as span events #1551
Add LogRecordProcessor to record event log records as span events #1551
Conversation
@breedx-splk, @LikeTheSalad - I'm happy to become a component owner for |
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.
Thanks. This is helpful and I didn't expect it to show up so soon! I had a few small questions/comments but this seems solid.
* <li>{@link LogRecordData#getObservedTimestampEpochNanos()} is mapped to span event attribute | ||
* with key {@code log.record.observed_timestamp} | ||
* <li>{@link LogRecordData#getSeverity()} is mapped to span event attribute with key {@code | ||
* log.record.severity_number} |
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.
Do you like log.record.severity_number
better than simply log.record.severity
? Maybe you're just trying to maintain consistency with the protos?
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.
Maybe you're just trying to maintain consistency with the protos?
Exactly... if / when the spec comes out and tell us how to translate between events and span events, we'll of course follow those instructions. But until then, the less opinionated we can be the better, and following the proto field naming is a way to avoid having an opinion.
processors/src/main/java/io/opentelemetry/contrib/eventbridge/EventToSpanEventBridge.java
Outdated
Show resolved
Hide resolved
processors/src/main/java/io/opentelemetry/contrib/eventbridge/EventToSpanEventBridge.java
Outdated
Show resolved
Hide resolved
@@ -56,6 +56,7 @@ components: | |||
processors: | |||
- LikeTheSalad | |||
- breedx-splk | |||
- jack-berg |
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.
Thanks for the help!
processors/src/main/java/io/opentelemetry/contrib/eventbridge/EventToSpanEventBridge.java
Show resolved
Hide resolved
processors/src/test/java/io/opentelemetry/contrib/eventbridge/EventToSpanEventBridgeTest.java
Show resolved
Hide resolved
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.
Haven't reviewed Java in a while but having filed the issue figured I'd give it a shot
processors/src/main/java/io/opentelemetry/contrib/eventbridge/EventToSpanEventBridge.java
Show resolved
Hide resolved
processors/src/main/java/io/opentelemetry/contrib/eventbridge/EventToSpanEventBridge.java
Outdated
Show resolved
Hide resolved
processors/src/main/java/io/opentelemetry/contrib/eventbridge/EventToSpanEventBridge.java
Outdated
Show resolved
Hide resolved
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.
Thanks!
if (!currentSpan.getSpanContext().equals(logSpanContext)) { | ||
return; | ||
} | ||
currentSpan.addEvent( |
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.
@jack-berg I just found this in the otep about events
It says span events will be deprecated in the API. I suppose it will still stay in SDK / proto because many backends only support trace signal. Is there a non-API approach for this processor? Or is it not worth reading too much into that for now?
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.
the purpose of deprecating won't be to remove, but rather to clearly signal when it is no longer the preferred approach
I don't think we will ever remove it since there's no plan for breaking changes
Resolves open-telemetry/opentelemetry-java#6880.
Supersedes open-telemetry/opentelemetry-java#6650.
The event {@link LogRecordData} is converted to attributes on the span event as follows:
event.name
attribute is mapped to span event nameLogRecordData#getTimestampEpochNanos()
is mapped to span event timestampLogRecordData#getAttributes()
are mapped to span event attributes, excludingevent.name
LogRecordData#getObservedTimestampEpochNanos()
is mapped to span event attribute with keylog.record.observed_timestamp
LogRecordData#getSeverity()
is mapped to span event attribute with keylog.record.severity_number
LogRecordData#getBodyValue()
is mapped to span event attribute with keylog.record.body
, as an escaped JSON string following the standard protobuf JSON encoding. NOTE: this results in an encoding that feels more verbose than necessary, but allows us to avoid having to define a new encoding.LogRecordData#getTotalAttributeCount() - LogRecordData#getAttributes().size()
is mapped to span event attributelog.record.dropped_attributes_count