-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[Prototype] log: Events support with minimal non-breaking changes #6018
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6018 +/- ##
=====================================
Coverage 82.1% 82.1%
=====================================
Files 273 273
Lines 23643 23653 +10
=====================================
+ Hits 19431 19440 +9
- Misses 3867 3868 +1
Partials 345 345 |
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.
I like this approach, as it is very simple and easy to understand. There is no breaking change that harasses users.
This is what OTel Rust has today. No separate method for EmitLog and EmitEvent. A single method named "emit", accepting a LogRecord. And LogRecord has an optional field "event_name". Tagging @lalitb too. |
This prototype adds event name support. Regarding compliance with https://github.com/open-telemetry/opentelemetry-specification/blob/50027a1036746dce293ee0a8592639f131fc1fb8/specification/logs/api.md#emit-an-event we could say that the
Logger.Emit
method defines both "Emit a Log Record" and "Emit an Event" functions at the same time.It adds
EventName
getter andSetEventName
setter to theRecord
struct andEventName
field toEnabledParameters
.These changes are NOT breaking from Go stability perspective. Adding methods to struct is defined as backwards-compatible. See: https://go.dev/doc/go1compat
It does not separate the events and bridged logs via a separate method but only adds an optional
EventName
field that the user need to set when emitting an event. The only problem with this design would be if the events and log records in the data model would have different fields. But AFAIK it is not intended. The intention is that log records and events have the same data model. Otherwise we would e.g. need to have different processors for events and logs which would end up with a new SDK and signal. If we want to use the logs exporters to send events then the events needs to share the same data model. As far as I understand, an event record is just a subset of the log record.It requires minimal changes from API implementations. It also requires less changes for anyone who needs to wrap interfaces. As rule of thumb, I find interfaces having less methods a better design sign. Doing everything (e.g. additional tests and docs) would require at most a few hundred LOC. It is worth noticing all existing logs SDK features (like
FilterProcessor
) are still working fine.It also worth mentioning that we already have some use cases why instrumentations would like to also emit regular log records which are not events: open-telemetry/opentelemetry-specification#4234. Therefore, it looks to be OK to give the users to be able to emit both log and event records.
In my opinion, for Go, the are more drawbacks than benefits in having separate methods for events (instead adding optional "event name" parameters). Thus I prefer this prototype over the other one: #6017.
I think that is also what https://github.com/open-telemetry/opentelemetry-rust and https://github.com/open-telemetry/opentelemetry-cpp may want to do. In .NET
ILogger
the EventId is also part of the "regular" log record emit methodLog
(see: here). Therefore, probably it would be also good for https://github.com/open-telemetry/opentelemetry-dotnet.CC @cijothomas, @marcalff
It is in my opinion it is the easiest, the most effort-less, the most intuitive, the less disruptive way to add Events support to Logs API and SDK.
It is also inline with the OTEP: Event Basics