open-telemetry / opentelemetry-specification

Specifications for OpenTelemetry
https://opentelemetry.io
Apache License 2.0
3.76k stars 890 forks source link

Make severity required to allow filtering by severity as early as possible #4159

Open bogdandrutu opened 4 months ago

bogdandrutu commented 4 months ago

Or alternative require that severity is provided as soon as possible in the https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/event-api.md#eventlogger, see for example an anti pattern of this in java where the severity can be provided after lots of other files in https://github.com/open-telemetry/opentelemetry-java/blob/main/api/incubator/src/main/java/io/opentelemetry/api/incubator/events/EventBuilder.java#L140 and that forces implementations to have to store all properties (attributes, etc.) until build time since severity can also be changed by calling setSeverity multiple times.

jack-berg commented 4 months ago

that forces implementations to have to store all properties (attributes, etc.) until build time since severity can also be changed by calling setSeverity multiple times.

Can you expand on this? If an implementation choses a builder pattern for constructing logs as java has done, how would that implementation avoid storing all properties?

cijothomas commented 4 months ago

Is the expectation that filtering is done at the OpenTelemetry level based on severity? (From my experience, such filtering is typically done inside the logging framework itself, so logs of undesired severity won't even reach OTel bridge layer)

pellared commented 4 months ago

Is the expectation that filtering is done at the OpenTelemetry level based on severity? (From my experience, such filtering is typically done inside the logging framework itself, so logs of undesired severity won't even reach OTel bridge layer)

Yes, we are expecting such use cases. E.g. when the minimum severity level could be set via OpenTelemetry Configuration File. Related PR: https://github.com/open-telemetry/opentelemetry-specification/pull/4020.

bogdandrutu commented 4 months ago

@pellared that is different because Logger is not a real logger :) (I said this many times, but I will repeat). For EventLogger which is a real Logger, users will write events, and all the logging API I've seen they do offer a "isEnabledForLevel/Severity()" but they also allow filtering by level/severity. See a good answer from @jack-berg https://github.com/open-telemetry/opentelemetry-java/issues/6603

The main idea is that an implementation of the EventLogger should be able to do minimal to zero work if filtering by severity is supported and this event will be filtered anyway. In order to do that with a "builder" pattern, what you can do is to pass the severity as an argument to the builder creation method, so that the implementation can return a no-op builder if that is the case and all calls will be no-ops. With the current API in Java at least this is impossible since the severity is not available when creating the builder, hence you need to return a real builder because you don't know if this event will or not be filtered.

bogdandrutu commented 4 months ago

Is the expectation that filtering is done at the OpenTelemetry level based on severity? (From my experience, such filtering is typically done inside the logging framework itself, so logs of undesired severity won't even reach OTel bridge layer)

For the EventLogger (which is a logging framework in my opinion or a thin wrapper over a logging framework) you need the level from the beginning, which is what every logging framework requires that first you provide the level and after everything else so that filtering can be done efficiently.

I expect that for the EventLogger implementation someone will use a logging framework to implement it, so hence you need the level immediately.

@cijothomas today based on the API in java (at least) https://github.com/open-telemetry/opentelemetry-java/blob/main/api/incubator/src/main/java/io/opentelemetry/api/incubator/events/EventLogger.java#L43 I need to create a real builder and accumulate all details for the event (create maps, etc.) because I don't know the level and cannot pass it to the logging framework, then only at the end when level is available I can do filtering, so a lot of unnecessary work before I can drop the data.

mtwo commented 2 months ago

Added to the Log / Event SIG project board

pellared commented 2 months ago

@bogdandrutu, what I described was just a clarification that allowing filtering by severity is needed also for logging library bridges (at least in Go). I think that nothing I wrote was against your proposal. Related issue: https://github.com/open-telemetry/opentelemetry-specification/issues/4208