-
Notifications
You must be signed in to change notification settings - Fork 893
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
Further decouple Events API from Logs/Logger #3086
Comments
@jack-berg @trask @jkwatson I opened this to continue the discussion that was happening on a closed PR. |
@open-telemetry/specs-logs-approvers @scheler @MSNev @alanwest @Aneurysm9 please review/comment. |
Do you imply then that we would reintroduce something like the API that was originally in the the Log SDK specification? This API was moved to to the API spec in #2768. opentelemetry-specification/specification/logs/logging-library-sdk.md Lines 68 to 84 in 5f772ee
I think moving it back to the SDK spec may be fine. One way or another appenders will need an API like this in order to call directly into the Log SDK. |
Conceptually, this would work. But it is basically creating a new first-class citizen (API) from an OpenTelemetry perspective... With the only real difference being that this new API doesn't define it's own transport mechanism. |
Yes, like I already wrote above "If we do this we essentially go back to the original design of the Log SDK (before Events API was added)." |
The only possible downside is that this could cause tighter coupling between the Event API and the Logs SDK than all people would (might) like. While having the Logs API means that external (not OpenTelemetry implementations) could work more smoothly with their own implementations |
We need the log API / SDK distinction for the same reasons we differentiate between API and SDK in other signals:
|
We talked in the 1/11 log SIG about the usage pattern of the pattern I've prototyped here, which is characterized by having separate Here is some example usage with separate
Eventually, once the event API is stable, it would be wrapped up in
If the event API is to delegate directly to the log API, it needs to be provided with a reference to an instance of the log API:
But each of these still requires that the caller of the event API be aware of the log API's existence, which conflicts with our stated goal that the log API should not be used by instrumentations other than log appenders. One alternative would be create
@Aneurysm9 / @tigrannajaryan / @MSNev do you all see other options that would allow the event API to directly call the log API but in a way that obscures the dependency from callers? |
and
appear to me to be the same thing, but in the former we'd have additional requirements on the logging SDK to implement not only the logging API but also the event emitter API. In the latter the event API/SDK are one thing and built in terms of the logging API. In neither case is the user creating an event emitter provider using the logging API. I would expect a logging SDK implementation that needed to also implement the event emitter API to also implement it in terms of the logging API, but now every SDK implementation needs to do that rather than being able to use the logging API in a single shared event emitter implementation. |
I disagree. If the contract is The difference is subtle but I think important. Any log surface area in the event API (even just FWIW, I think the last scenario I proposed (i.e. with the contract |
@Aneurysm9 can you rewrite your code examples above to not use the SDK so we can see what that would look like? I think @jack-berg @jkwatson and I are interested in the use case for instrumentation authors where the SDK is not available. |
I'm sorry if I was unclear. All of the samples I gave are assumed to be in the application initialization and not the instrumentation. This is covering how to obtain an
Does changing
This may be an option available to the Java implementation, but I don't think the
Instrumentation authors would be provided an I would maybe flip this around. What does a logging SDK that implements the |
Looking at Jack's PR, it seems that the SDK is already implementing the @Override
public Logger get(String instrumentationScopeName) {
return loggerComponentRegistry.get(
instrumentationNameOrDefault(instrumentationScopeName), null, null, Attributes.empty());
}
@Override
public EventEmitter get(String instrumentationScopeName, String eventDomain) {
return loggerComponentRegistry
.get(instrumentationNameOrDefault(instrumentationScopeName), null, null, Attributes.empty())
.withEventDomain(eventDomain);
} The second method there can be rewritten (logically, I don't Java so maybe there's a reason this is invalid): @Override
public EventEmitter get(String instrumentationScopeName, String eventDomain) {
LoggerProvider lp = this.get(instrumentationScopeName);
return lp.withEventDomain(eventDomain);
} |
I guess a tl;dr for my thoughts on the matter is this: Having the Logging SDK implement the Event Emitter API means that the Logging SDK needs to know that the Event Emitter API exists and forever couples the logging and event interfaces. Having the Event Emitter API/SDK implemented in terms of the Logging API means that only the Event Emitter implementations need to know about the Logging API but the Logging SDKs don't need to know that events are a thing at all. |
I think it's ok for the Event Emitter API to be implemented in terms of the Logging API, as long it's just an "implementation detail". I believe the Java folks' concern boils down to preserving the language introduced in #2966:
|
I think that quoted section is one sentence too long. I fully support and agree with this:
I think anything further is too far. Even if that entire paragraph is retained, I'd argue that an
An
The I'm worried that "the logging API must only ever be used to create Appenders" with an apparently implicit "for existing non-OTel logging systems" qualifier on "Appender" cuts off whole sets of potentially valid uses of the logging API that do not involve trying to replace log4j. There's a big difference between "end-users should not use the logging API" and "only log4j can use the logging API". |
👍 |
Here is my main goal: users of the Events API for instrumenting their code should not need to know it has anything to do with logging. The fact that we're using the logging datamodel to carry Events is definitely not a thing that users should know. From the instrumentation perspective, they should be considered as separate signals. If we make instrumentation authors have to understand that they're writing "special logs" then we will have not really built anything interesting or unique. |
I think we're in agreement here. I would propose that instrumentation authors receive an
Agreed to the extent that "users" means "instrumentation authors". Application authors will need to be aware of this as they will need to know where their events are going and how to control that.
Back to full agreement. Indeed, I think maybe we're not going far enough here in only proposing that there would be an Event API and not an Event SDK. If the flow looks like this then we'll only ever be able to emit events as logs:
If, instead, it looks like this:
then there are any number of types of transports that could back an event exporter and no coupling to the log API or SDK. Yes, that's more complicated but simple isn't an end in itself. |
@Aneurysm9 Yes, sorry, I meant instrumentation authors, rather than all users. I think we're in hearty agreement pretty much on all points. Whether or not we need a separate SDK is an interesting topic for discussion. I do think we should make it easy for vendors (and SDK configurers in general) to send events to a separate backend than standard logs. Whether that requires a separate SDK, or just a properly configurable LogProcessor, I'm not really sure. |
Based on the initial focus of the Event API being Clients (such as a browser) where the size of the code (in JS) is extremely important I would propose a slightly cut down version or at the very least a possible implementation which is just
Where the application would need to "know" that if they want to emit events then they would need to also load / initialize the Log SDK and if they don't then the Noop implementation (of Logs) would get used. I think this is effectively what we talked about in the Sig this morning where the Event SDK is like a "pass-thru" to the Log's API (ie. the EventEmitter would obtain the logging (Log API) instance in the same way as an appender implementation would by requiring the scope be passed to obtain the log emitter -- Or am I missing something? And then if/when we want to have |
I'm on board with has been discussed here and proposed by @MSNev. I think we were all in more agreement than it seemed. It meets the design requirements I care about: the event API has no dependency on the log API (it's the event SDK which delegates to the log API), and its possible to provide alternative implementations of the event API. I've updated my java prototype to reflect this design, and quite like the resulting code flow. Here's an updated version of the simple code flow I wrote out earlier in this thread:
No more need for the log SDK to implement multiple APIs, and it becomes super clear that the SDK |
So, being a picture-driven person I have to summarize the text above into this diagram :-) Pictured this way it is a bit of a smell and it hints that the Events SDK really is on a higher abstraction level than than Log SDK (and even the Log API), which probably is true. Events API and Log API are not really peers in our stack, similarly Events SDK and Log SDK are not peers. It may look better if Events API/SDK is lifted out of Otel boxes and is shown on the same level as the "Otel Instr". Somehow, again that feels more correct, the "events" use case is just one of the use cases that the Log API serves (along side the Bridge/Appender use cases). |
This seems right to me. As a Fun™ thought experiment imagine if we defined events for all operations in the trace and metric APIs. Then we could have a streaming trace/metrics SDK that used the events API and make some real spaghetti! |
I really really don't think it's a good idea to make "the events use case" be just one of the use-cases that the Log API serves. These things are semantically different, and again, we don't want an end-user-facing logs API. Just because we have chosen to transport events using the logs datamodel does not imply that events are logs. Only that we've seen a similarity in the data model. |
I disagree with this. Events are just specially shaped log records. That's what our spec says currently and I believe that's the message that we need to consistently send everywhere, including in the API/SDK design. |
I think this is the core of the disagreement. I do not think that events are "specially shaped log records". I think that's not how instrumentation authors should think about them, and I think it is not the right direction. They may be "syntactically" the same, but semantically they are separate and we should keep them separate. |
If you disagree with the spec please submit a PR with how you see it and have it merged. Until that happens I believe we should continue honouring what the spec says and what the SIG collectively agreed after many rounds of discussions. |
I unfortunately don't have the time or energy to fight this battle in a spec PR. Feel free to ignore my opinions. |
@jkwatson I think it would have been great if the spec clearly separates concerns for instrumentation authors and application authors. Assuming the instrumentation authors are concerned only with Events API note that it will not have any mention of Logs, based on @jack-berg's recent comment. Would that work? Application authors will remain aware of Events SDK, Logs SDK, Log API and the semantic conventions for Events, which mentions Events are represented using LogRecord data-model. I think you agreed on this with @Aneurysm9 above. |
Absolutely, but then @tigrannajaryan went another direction that is not in alignment with what @Aneurysm9 and I agreed on. |
Let's discuss in the SIG meeting today. I would like to at least make a conclusion on whether we have a Log API or no. |
Naming bikeshed: EventEmitter is a Node.js built-in API. It would be confusing in JavaScript if the API is named as EventEmitter but is not a Node.js EventEmitter instance. |
Discussed in Log SIG today. The conclusion is that we all agree Events API should not depend on Log SDK directly. This is what this issue was about and we consider that settled. The other topic that was discussed in this thread was whether Events SDK should depend on Log SDK or not. That is a separate issue and needs to be discussed separately. I think we need to delete from event-api.md this part and this issue can be closed:
|
I'm overseeing the migration from a traditional, log-based approach to instrumentation in my company's products to fully embracing OTEL. As part of this migration, there are two main "patterns" I've identified in the existing logs:
Since the logs that represent units of work are more or less equivalent to spans (in terms of the information contained in the log messages, the idea is to replace the statements that generate these logs with span-based instrumentation. For the second type of log, the idea is to transition as many of them to be "events", which is to say that we want to move away from freestyle, unstructured sentences, and towards using formal/qualified event names (as specified by the relevant OTEL spec) to express what's going on. {
"timestamp": 1668532427381458,
"severity.text": "ERROR",
"tags": {
"event.domain": "acme.feature_flags",
"event.name": "feature_flag_enabled",
"acme.feature_flag_name": "foo",
"exception.type": "com.acme.ServiceUnavailableException",
"exception.message": "Service unavailable (errorCode=430001)",
"thread.name": "main"
}
} I've stumbled upon this discussion, because the problem I'm facing is that I can't currently transition from this second type of log to emitting an event that carries the exact same information as the original log, because I do not have the ability to set a status. Despite what the Events spec says (both the currently published one, and the version that seems to have been updated to reflect the changes proposed in this issue), the Java implementation of the From official Event API spec:
From revised (but not yet merged) Event API spec:
From EventEmitter javadoc
This might seem like a small concern, but statuses are a pretty critical piece of metadata that I would like to preserve as part of the log-to-event migration. I also think that this is likely not going to be something that is specific to my use case: if events are really just specially-shaped log records, there should be a path available for users to set log-related fields when emitting an event if they wish to. That being said, I understand and completely agree with the desire to de-couple the event and logging APIs. I just hope that this can be accomplished while also retaining the ability to set log-related fields under some circumstances. |
Fixes #3086 ## Changes The Event API will be used by instrumentations to generate events. Currently, the API takes an instance of a logger as a direct dependency. This means that instrumentations would need to be aware of the Log API, which is in conflict with the intent of using the Log API only to bridge logging frameworks. Furthermore, each instrumentation would need to construct an instance of an EventLogger, which is also inconsistent with the pattern of using a globally-registered provider. This PR adds the `EventLoggerProvider` class to the API, making it consistent with other APIs.
Fixes open-telemetry#3086 ## Changes The Event API will be used by instrumentations to generate events. Currently, the API takes an instance of a logger as a direct dependency. This means that instrumentations would need to be aware of the Log API, which is in conflict with the intent of using the Log API only to bridge logging frameworks. Furthermore, each instrumentation would need to construct an instance of an EventLogger, which is also inconsistent with the pattern of using a globally-registered provider. This PR adds the `EventLoggerProvider` class to the API, making it consistent with other APIs.
The Events API as it is specified currently takes Logs API as a dependency. To generate events one has to obtain a Logger, then create an EventLogger associated with that Logger, then emit events via EventLogger's "Emit" method.
The comment thread here discusses the possibility of further decoupling Events API from the Logger API and having dedicated Events API and Events SDK (where internally Events SDK may share a codebase with Logs SDK).
Essentially instead of the current dependency/data flow of
User code -> Event API -> Log API -> Log SDK
the proposal is to haveUser code -> Event API -> Log SDK
orUser code -> Event API -> Event SDK
.There is a prototype implementation that demonstrates this approach for Java: open-telemetry/opentelemetry-java#5049
The overall structure of Events and Logs API/SDK packages and data flows eventually will look like this:
Open Questions:
The text was updated successfully, but these errors were encountered: