-
Notifications
You must be signed in to change notification settings - Fork 903
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
Allow LogProcessor to mutate log records #2681
Allow LogProcessor to mutate log records #2681
Conversation
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, just one non-blocking question on sync/async behavior.
@open-telemetry/specs-logs-approvers please take a look! |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Pinging @open-telemetry/specs-logs-approvers. Please take a look! Otel java has already implemented and published this change here. |
Resolves open-telemetry#2647. The simplest possible fix would have been to simply change `LogProcessor#emit` to accept `LogRecord` instead of `LogData`, but I took the opportunity to add some additional detail to make logs more consistent with spans where possible. Changes include: - Add TOC - Rename LogProcessor#emit to LogProcessor#onEmit. This is more consistent with spans (SpanProcessor#onStart, SpanProcessor#onEnd), and avoids confusion by being different than LogEmitter#emit. - Define LogRecord interface, accepted by LogEmitter#emit - Define Read/write LogRecord interface, accepted by LogProcessor#onEmit - Define Readable LogRecord interface, accepted in batches by LogExporter#export - Add LogExporter#forceFlush method (stubbed out for now) for consistency with SpanExporter#forceFlush
Resolves #2647.
The simplest possible fix would have been to simply change
LogProcessor#emit
to acceptLogRecord
instead ofLogData
, but I took the opportunity to add some additional detail to make logs more consistent with spans where possible.Changes include: