OTEP: Logger.Enabled

[pellared]

The main purpose of this OTEP is to have an agreement that the SDK needs both:

• a "hook" for Logger.Enabled for customization and flexibility via extending LogRecordProcessor with Enabled method
• new LoggerConfig fields to conveniently address the most popular, especially configuring the minimum severity level

The OTEP tries to elaborate how/why it should work well with (trace) sampling as well as Events API.

• New issues are going to be created if this OTEP is accepted and merged.
• Each "alternative" and "open question" would also have a new issue for tracking purposes (to explicitly reject or accept them).
• I will follow-up on the most important issues which looks to be
  • Add Enabled method to LogRecordProcessor (and describe how SDK evaluates the Logger.Enabled API)
  • Add minimum_severity_level field to LoggerConfig (and describe how SDK evaluates the Logger.Enabled API)

Fixes #4207

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[github-actions]

This PR was marked stale due to lack of activity. It will be closed in 7 days.

[cijothomas]

One thing I am not so sure about.. Do we expect OTel SDK to provide such control, mimicking what is often provided by the Logging libraries themselves? They often provide sophisticated mechanisms, and it may not be worth replicating everything in OTel...
We can explicitly state that our desire is to provide some control, but not necessarily matching/competing with existing logging libraries?

[pellared]

I do not expect the Logs API to be as user-friendly as the logging libraries popular for given language.

However, I think that the SDK (the logging backend) should be able handle different logging processing scenarios. E.g. sending verbose logs via user_events and warning logs via OTLP (probably I could come up with better examples). I heard that someone wanted to send non-event log records and event records to different destinations. Moreover, there is the following question in the Events Basics OTEP:

How to support routing logs from the Logs API to a language-specific logging library while simultaneously routing logs from the language-specific logging library to an OpenTelemetry Logging Exporter?

I see logging libraries as logs frontend and Logs SDK as logs backend. Logs API is the thing which can bridges between the frontend and backend.

Side note: I think that even without this use case we would get the same design.

[pellared]

I moved it as the last use-case given that it may be seen as the least important. a4257ef

[pellared]

Let me know if the current way it is described looks good overall.

[lmolkova]

I think we'll (eventually) have more config options for severity, sampling, and other things, e.g. a few extra scenarios I can think of:

• minimum level for this event name
• maximum level to sample at (e.g. you only want to sample info and lower)
• minimum level to record exception stack traces at
• sampling on/off rate for this event name
• sample based on spans, but also record all logs without parent span (e.g. startup logs are quite important)
• throttling config (record this event name, but at most X times per time unit)
• ...

We'll need to think how to structure them on LoggerConfig API so it's extendable but only introduce options we have an immediate need for).

E.g. I think we may need something like LoggerConfig.sampling_strategy = always_on | span_based instead of disabled_on_sampled_out_spans.

Also we'd need to consider having a callback in addition to pure declarative config (e.g. for cases like sampling based on spans, but also startup logs are all in). For this we should consider having a sampler-like interface which would have implementations similar to tracer sampler and then we might want to steal declarative config from tracer_provider.sampler.

---

TL;DR:

• let's figure out what we should do first - declarative config vs api extensibility
• let's come up with extendable options

[pellared]

We'll need to think how to structure them on LoggerConfig API so it's extendable but only introduce options we have an immediate need for).

Adding a field is a backwards compatible change. I agree that we should add new fields (options) only if we have a big demand for it.

we'd need to consider having a callback in addition to pure declarative config

For more ad I think the LogRecordProcessor comes into play. It should handle the most advanced scenarios.

I will do my best to describe it "Future possibilities" section.

let's figure out what we should do first - declarative config vs api extensibility

IMO extensibility (LogRecordProcessor.Enabled) is more important as it can also solve the (3) and (4) use cases even without the config extensions.

let's come up with extendable options

I would like to know if there is a use cases that the proposal of adding LogRecordProcessor.Enabled would not solve.

[pellared]

Also we'd need to consider having a callback in addition to pure declarative config

I added "Alternatives: Dynamic Evaluation in LoggerConfig"

I think we'll (eventually) have more config options for severity, sampling, and other things, e.g. a few extra scenarios I can think of:

I added "Future possibilities: Extending LoggerConfig

We would need to closer look at the alternatives when prototyping and during the Development phase of the spec.

[pellared]

@lmolkova, is there anything more to address regarding this discussion?

[lmolkova]

what would you expect processors to do inside Enabled? If this is a delegating processor, would it call the underlying one? When I'm implementing a processor that does filtering, I should assume it's in the certain position in the pipeline?

I.e. it sounds like there should be just one processor in the pipeline that decides if log should be created. If so, should it be a processor responsibility at all or can we call this component LogFilter, LogSampler, etc and make it a configurable behavior independent of the processor?

If this component returns true, all processing pipelines would get log record and can drop it if they are filtering too.
If it returns false, none of them should. This is the same as you have outlined in the otep, just extracts filtering from processing.

[lmolkova]

Some examples of logging filters:

Here's a good old .NET ILogger filter

builder.Logging.AddFilter((provider, category, logLevel) =>
{
    if (provider.Contains("ConsoleLoggerProvider")
        && category.Contains("Controller")
        && logLevel >= LogLevel.Information)
    {
        return true;
    }
    else if (provider.Contains("ConsoleLoggerProvider")
        && category.Contains("Microsoft")
        && logLevel >= LogLevel.Information)
    {
        return true;
    }
    else
    {
        return false;
    }
});

or good old log4j

private void updateLoggers(final Appender appender, final Configuration config) {
    final Level level = null;
    final Filter filter = null;
    for (final LoggerConfig loggerConfig : config.getLoggers().values()) {
        loggerConfig.addAppender(appender, level, filter);
    }
    config.getRootLogger().addAppender(appender, level, filter);
}

I.e. composition instead of inheritance - when the logging is configured, each appender/provider pipeline gets one dedicated filter independent of the appender/provider logic.

[pellared]

what would you expect processors to do inside Enabled? If this is a delegating processor, would it call the underlying one? When I'm implementing a processor that does filtering, I should assume it's in the certain position in the pipeline?

Depends on the processor.

1. If this is an exporting processor, e.g. for user_events then Enabled, returns results only for itself. Example: https://github.com/open-telemetry/opentelemetry-rust-contrib/blob/30a3f4eeac0be5e4337f020b7e5bfe0273298858/opentelemetry-user-events-logs/src/logs/exporter.rs#L158-L165. Use case (5).
2. For a filtering processor it needs to wrap some other processor for which it applies the filtering. Then it makes it possible to have two batching processors with different filters. Example: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/a5391050f37752bbcb62140ead0301981af9d748/processors/minsev/minsev.go#L74-L84 Use case (6).

I.e. it sounds like there should be just one processor in the pipeline that decides if log should be created. If so, should it be a processor responsibility at all or can we call this component LogFilter, LogSampler, etc and make it a configurable behavior independent of the processor?

If this component returns true, all processing pipelines would get log record and can drop it if they are filtering too. If it returns false, none of them should. This is the same as you have outlined in the otep, just extracts filtering from processing.

I think is that it does not suites well (5) nor (6) use cases.
For (5) would someone configuring an user_events exporter would have to configure a filterer as well as processor?
For (6) it would not be even possible to e.g. allowing distinct minimum severity levels for different processors as the filters would run before the processors.
The other way is that the processors themselves would accept a filterer, but I do not think we want to Logs SDK to become https://logging.apache.org/log4j/2.x/manual/architecture.html (which by the way does not offer an Enabled API).

I.e. composition instead of inheritance - when the logging is configured, each appender/provider pipeline gets one dedicated filter independent of the appender/provider logic.

Filtering processor mentioned before wraps an existing processor so it is following the principle to favor composition over inheritance.

I added "Alternatives: Separate LogRecordFilterer Abstraction".

We would need to closer look at the alternatives when prototyping and during the Development phase of the spec.

[lmolkova]

For (5) would someone configuring an user_events exporter would have to configure a filterer as well as processor?

The trade-off:

1. most processors would not need IsEnabled - processors today are for processing LogRecords. The two processor we require (simple and batch) would not have it. If IsEnabled is an API on the processor, from now on everyone would need to think twice whether they need to implement it and what would delegating/composite processors need to do ("any of" or "all of"?)
2. a few processors (it's really exporters, not processors) that would need IsEnabled would have a bit more complicated config.

I would pick 2 as the better option.

For (6) it would not be even possible to e.g. allowing distinct minimum severity levels for different processors as the filters would run before the processors.

The filter can be "any of" (if any of the filters returns true, record is passed to the processor) .
Processor implementations can absolutely support additional filtering, but not through base interface API.

[pellared]

most processors would not need IsEnabled

Similarly most exporters do not need ForceFlush and Shutdown.
If it is "not needed" then it should return true. It could be the default implementation (this is how ForceFlush and Shutdown is designed in some languages). I think that the current proposal follows existing patterns.

If IsEnabled is an API on the processor, from now on everyone would need to think twice whether they need to implement it and what would delegating/composite processors need to do

I think that in this context it is better that the developer would need to think how IsEnabled should be implemented.

Processor implementations can absolutely support additional filtering, but not through base interface API.

Then each processor would have to provide its own filtering API. I think it would make the user experience worse.

[dashpole]

Can processors that need additional functionality implement both interfaces? Or is that not possible in all languages?

[pellared]

Can processors that need additional functionality implement both interfaces? Or is that not possible in all languages?

Do you mean something that is currently described in "Trade-offs and mitigations" section?

For some langagues extending the LogRecordProcessor may be seen as breaking.
For these languages implementing LogRecordProcessor.Enabled must be optional.
The SDK LogRecordProcessor must return true if Enabled is not implemented.
This approach is currently taken by OpenTelemetry Go.

Is it what you have in mind?
If so then do you think it should be the recommended approach?
I would say that it is a language/SIG specific decision because of the trade-offs of either approaches.

On the other hand, based on the feedback, I think that we should indeed try to coin define a new filterer abstraction (interface) that could be used as an option for the logger provider and processors. Defining it as a separate abstraction may result in better user experience in most scenarios.

My plan is to revisit, refresh and enhance a prototype I did some time ago: open-telemetry/opentelemetry-go#5825

[pellared]

After some thoughts and chat with @lmolkova, I think we can start with LogRecordProcessor.Enabled.

I still plan to create an issue for "Separate LogRecordFilterer Abstraction". If we will get feedback that LogRecordProcessor.Enabled is bad then we can switch to it. I can still make a prototype for the alternative design. We can always revisit the design during Development phase before going stable after gathering feedback and more hand-on experience.

[pellared]

I did my best to address it via 1a977c6