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.

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

Merged
merged 29 commits into from
Nov 1, 2024
Merged

Conversation

tedsuo
Copy link
Contributor

@tedsuo tedsuo commented Oct 15, 2024

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

specification/logs/bridge-api.md Outdated Show resolved Hide resolved
specification/logs/bridge-api.md Outdated Show resolved Hide resolved
specification/logs/bridge-api.md Outdated Show resolved Hide resolved
specification/logs/bridge-api.md Show resolved Hide resolved
@MSNev
Copy link
Contributor

MSNev commented Oct 15, 2024

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
#4252

-- Going to reclose my PR in preference of smaller steps like this PR.

@tedsuo
Copy link
Contributor Author

tedsuo commented Oct 15, 2024

@MSNev there are several closed and stale issues on this subject, can we just continue with this one?

@tedsuo
Copy link
Contributor Author

tedsuo commented Oct 15, 2024

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.

@tedsuo
Copy link
Contributor Author

tedsuo commented Oct 15, 2024

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 EmitEvent. But there needs to be a base method which is separate from the log bridge method, as it gives users the opportunity to intercept all of the Otel native instrumentation separately from any legacy logging that may be coming through the bridge.

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 EmitEvent to the logger is trivial, it is not burdensome overhead for implementors to have this one function be required.

@lmolkova
Copy link
Contributor

lmolkova commented Oct 15, 2024

It is fine if languages want to add additional sugar for making events on top of EmitEvent. But there needs to be a base method which is separate from the log bridge method, as it gives users the opportunity to intercept all of the Otel native instrumentation separately from any legacy logging that may be coming through the bridge.

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.

@tedsuo
Copy link
Contributor Author

tedsuo commented Oct 15, 2024

@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.

Copy link
Contributor

@lmolkova lmolkova left a 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.

specification/logs/bridge-api.md Outdated Show resolved Hide resolved
@tedsuo
Copy link
Contributor Author

tedsuo commented Oct 16, 2024

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.

@pellared pellared changed the title Add EmitEvent to Log API Add EmitEvent to Logs API Oct 16, 2024
@pellared
Copy link
Member

Can you add a changelog entry?

@tedsuo
Copy link
Contributor Author

tedsuo commented Oct 29, 2024

fyi, the prototype is here: open-telemetry/opentelemetry-java#6761

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Robert Pająk <[email protected]>
@lmolkova
Copy link
Contributor

It seems we have a consensus, so I'm going to merge this PR tomorrow (unless new feedback comes up).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.