Closed tigrannajaryan closed 8 months ago
@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.
should we eliminate the "Log API" package and have Log Library bridges/appenders call directly into Log SDK?
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.
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.
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.
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
Given this, should we eliminate the "Log API" package and have Log Library bridges/appenders call directly into Log SDK? If we do this we essentially go back to the original design of the Log SDK (before Events API was added).
We need the log API / SDK distinction for the same reasons we differentiate between API and SDK in other signals:
opentelemetry-java-instrumentation
have grown to depend on the API / SDK distinction. When there was only a log SDK available, the java agent was forced to introduce its own appender API anyway to avoid adding log SDK classes to the bootstrap classloader. More details available here. It was a good day when we introduced the log API and were able to rip out a bunch of awkward code from the opentelemetry-java-instrumentation
. 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 EventEmitterProvider
and EventEmitter
. The motivation for this is to allow callers of the event API to not have to be aware of the log API, but there was some debate over whether or not this separation is needed to hide users from the log API.
Here is some example usage with separate EventEmitterProvider
/ EventEmitter
:
// Instrumentation code, which only depends on the API
void initializeInstrumentation(EventEmitterProvider) {
EventEmitter eventEmitter = eventEmitterProvider.get("my-scope", "my-event-domain);
eventEmitter.emit("my-event-name", Attributes.builder().put("foo", "var").build());
// ...
}
..
// Application code, which initializes the SDK and wires up instrumentation
public static void main(String[] args) {
// Configure SDK
EventEmitterProvider sdkEventEmitterProvider = SdkLoggerProvider.builder()
// ..omitted for brevity
.build();
// Wire up instrumentation
initializeInstrumentation(sdkEventEmitterProvider);
}
Eventually, once the event API is stable, it would be wrapped up in OpenTelemetry
, so the code would shift to:
// Instrumentation code, which only depends on the API
void initializeInstrumentation(OpenTelemetry openTelemetry) {
EventEmitter eventEmitter = openTelemetry.getEventEmitterProvider().get("my-scope", "my-event-domain);
eventEmitter.emit("my-event-name", Attributes.builder().put("foo", "var").build());
// ...
}
..
// Application code, which initializes the SDK and wires up instrumentation
public static void main(String[] args) {
// Configure SDK
OpenTelemetry openTelemetry = OpenTelemetrySdk.builder()
.setEventEmitterProvider(SdkLoggerProvider.builder()
// ..omitted for brevity
.build())
.build();
// Wire up instrumentation
initializeInstrumentation(openTelemetry);
}
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:
// Pass a Logger when creating EventEmitter, as is defined today in the Event API spec
EventEmitter eventEmitter = EventEmitter.create(loggerProvider.get("my-scope"), "my-event-domain");
// ... Alternatively pass a LoggerProvider when creating a EventEmitterProvider
EventEmitterProvider eventEmitterProvider = EventEmitterProvider.get(loggerProvider);
EventEmitter eventEmitter = eventEmitterProvider.get("my-scope", "my-event-domain");
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 EventEmitterProvider
with an instance of LoggerProvider
indirectly, by means of an OpenTelemetry
instance:
OpenTelemetry openTelemetry = // ... initialize or obtain OpenTelemetry instance
// EventEmitterProvider.get accesses openTelemetry.getLoggerProvider(), effectively hiding the log API dependency from users
EventEmitterProvider eventEmitterProvider = EventEmitterProvider.get(openTelemetry);
EventEmitter eventEmitter = eventEmitterProvider.get("my-scope", "my-event-domain");
@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?
EventEmitterProvider sdkEventEmitterProvider = SdkLoggerProvider.builder()
// ..omitted for brevity
.build();
and
LoggerProvider loggerProvider = SdkLoggerProvider.builder()
// ..omitted for brevity
.build();
EventEmitterProvider eventEmitterProvider = EventEmitterProvider.get(loggerProvider);
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.
In neither case is the user creating an event emitter provider using the logging API.
I disagree. If the contract is EventEmitterProvider.get(LoggerProvider)
, an instrumentation author is aware of and must use the log API. In the former example where SdkLoggerProvider
implements both LoggerProvider
and EventEmitterProvider
, its the application owner that is aware that events and logs use the same SDK.
The difference is subtle but I think important. Any log surface area in the event API (even just LoggerProvider
) is a potential source of confusion for instrumentation authors who read that the log API is not meant to be used outside appenders.
FWIW, I think the last scenario I proposed (i.e. with the contract EventEmitterProvider.get(OpenTelemetry)
) allows the event API to delegate directly to the log API, but without any log API surface area in the event API. I hadn't thought of it before and it doesn't seem terrible to me. Curious what @jkwatson / @mateuszrzeszutek / @trask think.
@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.
In neither case is the user creating an event emitter provider using the logging API.
I disagree. If the contract is
EventEmitterProvider.get(LoggerProvider)
, an instrumentation author is aware of and must use the log API. In the former example whereSdkLoggerProvider
implements bothLoggerProvider
andEventEmitterProvider
, its the application owner that is aware that events and logs use the same SDK.
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 EventEmitterProvider
that can be provided to instrumentation if the default is not desired.
The difference is subtle but I think important. Any log surface area in the event API (even just LoggerProvider) is a potential source of confusion for instrumentation authors who read that the log API is not meant to be used outside appenders.
Does changing EventEmitterProvider
to EventAppenderProvider
change this? I'm having a real hard time with there being a special API that can only be used by special things even when other things are trying to do exactly the same operations.
FWIW, I think the last scenario I proposed (i.e. with the contract EventEmitterProvider.get(OpenTelemetry)) allows the event API to delegate directly to the log API, but without any log API surface area in the event API. I hadn't thought of it before and it doesn't seem terrible to me.
This may be an option available to the Java implementation, but I don't think the OpenTelemetry
object used here is part of the spec or implemented in other languages.
@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.
Instrumentation authors would be provided an EventEmitterProvider
and use that to obtain an EventEmitter
. They never know whether it is backed by an SDK or not. The application author responsible for giving the instrumentation the EventEmitterProvider
is the only one who interacts with the SDK. This is exactly how every other signal is handled.
I would maybe flip this around. What does a logging SDK that implements the EventEmitterProvider
interface look like if it isn't just using the logging API it implements? If it is just using the logging API it implements then why does every SDK implementation need to re-implement that wrapping?
Looking at Jack's PR, it seems that the SDK is already implementing the EventEmitterProvider
in terms of the LoggerProvider
:
@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.
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.
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:
Note: this document defines a log backend API. The API is not intended to be called by application developers directly. It is provided for logging library authors to build Appenders, which use this API to bridge between existing logging libraries and the OpenTelemetry log data model.
I believe the Java folks' concern boils down to preserving the language introduced in #2966:
Note: this document defines a log backend API. The API is not intended to be called by application developers directly. It is provided for logging library authors to build Appenders, which use this API to bridge between existing logging libraries and the OpenTelemetry log data model.
I think that quoted section is one sentence too long. I fully support and agree with this:
Note: this document defines a log backend API. The API is not intended to be called by application developers directly.
I think anything further is too far.
Even if that entire paragraph is retained, I'd argue that an EventEmitter
is an Appender
:
An Appender implementation can be used to allow emitting logs via OpenTelemetry LogRecordExporters.
An EventEmitter
is used to emit events as logs.
Appenders can be created in OpenTelemetry language libraries by OpenTelemetry maintainers...
The EventEmitter
implementation built in terms of the logging API would be created in OTel language libraries by OTel maintainers.
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".
end-users should not 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.
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.
I think we're in agreement here. I would propose that instrumentation authors receive an EventEmitterProvider
and use it to construct an EventEmitter
that they use to emit events. No awareness of underlying transport.
The fact that we're using the logging datamodel to carry Events is definitely not a thing that users should know.
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.
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.
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:
instrumentation -> Event API -> Log SDK
If, instead, it looks like this:
instrumentation -> Event API -> Event SDK -> Event Exporter -> Log API -> Log SDK
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
instrumentation -> Event API -> Event SDK (using Logs) -> Log API
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 any number of types of transports that could back an event exporter
then this is a specific Event SDK implementation of the Event API (or an extension of the base (initial) one.
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:
// Instrumentation code, which only depends on the API
void initializeInstrumentation(EventEmitterProvider) {
EventEmitter eventEmitter = eventEmitterProvider.get("my-scope", "my-event-domain);
eventEmitter.emit("my-event-name", Attributes.builder().put("foo", "var").build());
// ...
}
..
// Application code, which initializes the SDK and wires up instrumentation
public static void main(String[] args) {
// Configure SDK
LoggerProvider sdkLoggerProvider = SdkLoggerProvider.builder()
// ..omitted for brevity
.build();
EventEmitterProvider eventEmitterProvider = SdkEventEmitterProvider.create(sdkLoggerProvider);
// Wire up instrumentation
initializeInstrumentation(eventEmitterProvider);
}
No more need for the log SDK to implement multiple APIs, and it becomes super clear that the SDK EventEmitterProvider
delegates to LoggerProvider
.
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).
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.
Just because we have chosen to transport events using the logs datamodel does not imply that events are logs.
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.
Just because we have chosen to transport events using the logs datamodel does not imply that events are logs.
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.
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 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.
I do not think that events are "specially shaped log records". I think that's not how instrumentation authors should think about them..
@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.
I do not think that events are "specially shaped log records". I think that's not how instrumentation authors should think about them..
@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:
logger - the delegate Logger used to emit Events as LogRecords.
TLDR: It seems as if EventEmitter should probably allow log-related fields to be set. That, or the spec should be updated to make it clear that those fields are not meant to be set those log-related fields.
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:
1) Logs that represent units of work, ex:
2022-11-15 17:13:47.381 +0000 (user@mail.com,182436025) pool-21-thread-1 myservice: INFO com.acme.MyApiController - requestDuration=200ms, status=200, cached=true
2) Logs with human-readable messages, which potentially contain interporlated value, ex:
2022-11-15 17:13:47.381 +0000 main : ERROR FeatureFlagManager - Failed to enable feature flag "foo": com.acme.ServiceUnavailableException: Service unavailable (errorCode=430001)
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 EventLogger
does not currently seem to accept a log record, or any log record-related fields.
From official Event API spec:
event_name - the Event name. This argument MUST be recorded as a LogRecord attribute with the key event.name. Care MUST be taken by the implementation to not override or delete this attribute while the Event is emitted to preserve its identity. logRecord - the LogRecord representing the Event
From revised (but not yet merged) Event API spec:
event_name
- the Event name. This argument MUST be recorded as aLogRecord
attribute with the keyevent.name
. Care MUST be taken by the implementation to not override or delete this attribute while the Event is emitted to preserve its identity.event
- the LogRecord representing the Event.
From EventEmitter javadoc
// Spec says I should be able to set body, serverity text/number, etc., but only attributes are available
eventEmitter.emit("my-event", Attributes.builder()
.put("key1", "value1")
.put("key2", "value2")
.build())
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.
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: https://github.com/open-telemetry/opentelemetry-java/pull/5049
The overall structure of Events and Logs API/SDK packages and data flows eventually will look like this:
Open Questions: