-
Notifications
You must be signed in to change notification settings - Fork 894
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 EmitEvent to Logs API #4259
Conversation
I create a draft for this last week, but I've had is closed so that we could discuss in the SIG meeting today -- I've just reopened it -- Going to reclose my PR in preference of smaller steps like this PR. |
@MSNev there are several closed and stale issues on this subject, can we just continue with this one? |
Just fyi, this PR only intends to add the EmitEvent as a method to the Logger – as we agreed upon – along with the description of this method. Once approved, we can follow up with any needed changes to the SDK. We probably want to add more details in the spec that define the difference between the Bridge functions and the Logging functions. but I'd like to do that as a separate PR because it sounds like a bikeshed. Likewise, I would like to make separate PR deprecating or removing the EventProvider. So for this PR, please focus on the method itself. The definition matches the current definition on the EventLogger, which was already approved. |
Another clarification: why is EmitEvent a method, and not sugar? It is fine if languages want to add additional sugar for making events on top of Ensuring that API methods can be intercepted before any processing is a core part of our API/SDK design – we assume that our implementation will not be correct for some portion of our users, and they should be allowed to replace our implementation with their own. There isn't an advantage to removing this interception point – adding |
I'd like to challenge that - users can create events using 3rd party loggers and events would come through bridge API. I don't understand how and why we can separate events coming through events API from events that came through the bridge. Events can be uniformly separated from logs in the processor on the SDK level regardless of their source. The whole scenario of separation seem to be an edge case or temporary back-compat solution for span-events, yet we're using it to guide long-term API design decisions. |
@lmolkova if users have some reason for sinking events via the log bridge, that is fine. But sinking logs and writing instrumentation are two different intents. We should have a separate method, one for each task. We should not assume that some users will struggle if they cannot isolate Otel instrumentation. Since we have two very separate intents, we support our users best by trying to keep these intents separate in our API. That is why we always have clean API/SDK separation. We assume that there is a "black swan" out there, even if we have never seen it, and that at least some users will want to provide their own implementation. |
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 believe this PR does not change status quo and is just moving Events API under logger.
I think we still need to have a discussion on how much leeway SIGs should have when (if) implementing this API (that's tracked here #4193) but given that this API is under development we can do it as a follow up.
I've updated the PR to organize the functions on the logger by operation type – bridge, instrumentation, and helper operations. This file should be renamed from "bridge-api" to "api." I can do that as part of this PR once everyone has had a look at the other changes. |
Can you add a changelog entry? |
fyi, the prototype is here: open-telemetry/opentelemetry-java#6761 |
52101d6
to
2a52859
Compare
Co-authored-by: Robert Pająk <[email protected]>
It seems we have a consensus, so I'm going to merge this PR tomorrow (unless new feedback comes up). |
Continuing the work of implementing OTEP 265, this PR adds the EmitEvent method to the Log API.
Deprecating the experimental Event API and SDK can be handled in a follow up PR.
Java prototype: open-telemetry/opentelemetry-java#6761