Define Enabled parameters for Logger

[pellared]

Follows #4020

During 7/5/24 OTel Specification SIG meeting it was decided to move the discussion about parameters to a separate PR; a little cleaned up transcript:

We're essentially trying to foresee with this is like what arguments will need to be present, for this Enabled method. Is it just severity? Is it no arguments at all? Are we going to want to evolve the arguments over time. I don't have the answers to those questions.
I wonder if we can do something [...] just to progress this because there's kind of [...] chicken in the eggs problems here. [...] we want to get this merged. [...]
This API should be structured so that the arguments you pass to it are evolvable. [...] we can leave ourselves room to evolve it from multiple standpoints. There's explicit text that says that the arguments can change and it's experimental.
I think I'd feel good about it then, because then we could [...] get this valuable thing in now [...]
[...]
And maybe we open some sort of issue [...] as a prerequisite for stability, we should [...] more thoughtfully consider what arguments are required for this.

This is needed for stabilization of Logger.Enabled in the spec which is need for the stabilization of Go Logs API.

Related issue: open-telemetry/opentelemetry-go#5769

Implementation in Go: open-telemetry/opentelemetry-go#5791

Changes

Add a minimal (and maybe even sufficient?) set of parameters for Logger.Enabled:

• Context is needed for allowing "context related" behavior. For instance, it enables to user to disable logging for some key/value in the context has been set (example). It can also used to see if the call is within a span.
• SeverityLevel is needed as it is common to configure the minimum level for logging (e.g. https://pkg.go.dev/go.opentelemetry.io/contrib/processors/minsev).
  Moreover, the https://pkg.go.dev/log/slog#Handler (Go standard library structured logging), https://pkg.go.dev/go.uber.org/zap/zapcore#LevelEnabler (popular Go structured logging library), https://pkg.go.dev/github.com/go-logr/logr#LogSink (opinionated common logging abstraction) allow creating a bridge/hook/sink that is checking if a logger is enabled and it accepts the logging level as an argument.

Add guidelines/rules for the Enabled parameters.

[jack-berg]

These parameters were included in the original PR, and we had a (long) conversation about their values here.

In a nutshell, my critique is that the SDK has to provide an implementation of this Logger#enabled method, but there is currently nothing in the SDK spec which would allow the Logger#enabled to do anything differently based on the context / severity arguments.

In its current form, this is half of a feature. SDKs will always response with Logger#enabled based on the logger's associated LoggerConfig.disabled, ignoring the severity / context arguments.

The disconnect appears to be based on the go implementation's concept of a FilterProcessor which is able to express whether or not it needs to process log records based on severity. This concept does not exist in the spec, and would need to for this concept to make sense.

[pellared]

In a nutshell, my critique is that the SDK has to provide an implementation of this Logger#enabled method, but there is currently nothing in the SDK spec which would allow the Logger#enabled to do anything differently based on the context / severity arguments.

Cannot this be proposed as a separate PR?

In Go SIG, we are indeed experimenting how to best model it on the SDK level. We have an experimental feature, but we would like to have some more feedback before we propose something to the specification.

I think we can separately work on specifying the API and how SDK is implementing the API.

In my opinion, designing and implementing the SDK implementation would be harder and more opinionated. Therefore, I think it would be better to separate those into distinct PRs.

I agree that not having any specification on the SDK level may be seen as a blocker of making it stable. On the other hand, there are already APIs which are not defined how the SDK should handle them like #4160.

Notice that this section is still in "Development".

[jack-berg]

Cannot this be proposed as a separate PR?

I'm fine splitting this out in a way that allows iteration. But we need a way to not loose track of this - maybe a TODO in the spec and a new issue upon merging? Resolving this discrepancy between the API and SDK should be a blocker to stability.

[pellared]

Cannot this be proposed as a separate PR?

I'm fine splitting this out in a way that allows iteration. But we need a way to not loose track of this - maybe a TODO in the spec and a new issue upon merging? Resolving this discrepancy between the API and SDK should be a blocker to stability.

Good idea. I created:

• Specify how Logs SDK implements Enabled #4207
• Stabilize Logs Enabled #4208

Feel free to update description or even create more issues.

Tracking issues created

[trask]

may also want to consider blocking stability on whether we want to include "event name" in the enabled parameters, e.g. see @lmolkova's open-telemetry/oteps#265 (comment)

[pellared]

may also want to consider blocking stability on whether we want to include "event name" in the enabled parameters, e.g. see @lmolkova's open-telemetry/oteps#265 (comment)

If you want you can add it to #4208.

However, I do not think that it should block the stability of Enabled as:

1. Adding a parameter would not be a breaking change
2. I feel that there is no conclusion if we want to add an "event name" do the LogRecord model or if we just want to use an event.name log attribute for it. It may be better to add "attributes" to enabled parameters. I think we would rather use just an attribute for that. See Event basics oteps#265 (comment)

[reyang]

1. Adding a parameter would not be a breaking change

💯

2. I feel that there is no conclusion if we want to add an "event name" do the LogRecord model or if we just want to use an event.name log attribute for it. It may be better to add "attributes" to enabled parameters. I think we would rather use just an attribute for that. See Event basics oteps#265 (comment)

There might be more to consider (definitely not for this PR):

1. Comparing strings could be very inefficient, many systems use integers (e.g. Event Id = 1001) in combination with user-friendly names.
2. For platforms with very limited resource or mission critical software (e.g. IoT device, kernel drivers), folks might want to just emit the integer, then enrich the data in the pipeline/backend using the integer -> string name mapping.

[pellared]

Here (Proposal A) is an idea how the SDK can implement Logger.Enabled.

[pellared]

Here (Proposal B) is a second (I think better) idea how the SDK can implement filtering which will affect not only Logger.Enabled but also Logger.Emit. It is more like sampling in tracing SDK.

[pellared]

Suggested change

	Additional optional parameters can be added in the future, therefore,
	the API MUST be structured in a way for these parameters to be added.
	It SHOULD be possible to distinguish between an unspecified parameter value from
	a parameter value set explicitly to a valid default value of given type
	(e.g. distinguish unspecified attributes for empty attributes). The exception
	from this rule is when the default value of given type is not seen as a valid
	value like 0 for [Severity Number](./data-model.md#field-severitynumber).

By @jack-berg in #4207 (comment):

I'm suspicious of the idea of introducing the concept of an expandable set of EnabledParameters, since requiring users to create a new struct / object to check if code should proceed to emitting a log seems non-optimal from a performance and ergonomics standpoint. I understand the desire to want to retain flexibility to evolve the API, but I think getting the arguments right in the initial stable version and not trying to retain the ability to evolve is likely to produce the most ergonomic API.

I think that in many languages expandable set of parameters for Logger.Enabled with acceptable performance and ergonomics standpoint can be achieved by using optional function/method arguments. But for Go you are correct, having Logger.Enabled to accept only context and severity would make the API more ergonomic.

I am worried that is may be hard to get the arguments right in the initial version. For instance, should we allow filtering based on attributes or body? Do we want to close the doors for such possibility? On the other hand, from my experience having only context and severity would cover 99% of use cases and having a better ergonomics and easier design is also desirable.

[cijothomas]

On the other hand, from my experience having only context and severity would cover 99% of use cases

Agree on this. Most logging frameworks I am familiar with provide IsEnabled/equivalent by using minimal arguments, and in particular, those arguments that are known easily (static in some languages like Rust). The usual goal of IsEnabled is optimize performance, so minimal arguments meets the goal here.
Examples:
.NET ILogger allows filtering based on Severity, Scope only.
Rust's tracing (logging solution!), only allows filtering based on Severity,Scope,Name (event name)

For filtering based on attributes/body (or anything that is dynamic in nature) - these are typically done not for Perf savings, but for saving backend storage costs. These are better handled by LogRecordProcessor (in-proc) or Collector (out-of-proc).

[pellared]

only allows filtering based on Severity,Scope,Name (event name)

The scope will be known by the SDK - no need to add it to Enabled as the parameter.

Question if we would need event name as a parameter. @cijothomas do you know if this is frequently used and what are the use cases when it is used? I think a prerequisite for it would be to to add it as a field to LogRecord (it could be defined in the same way as Body and named ID or Name).

[cijothomas]

The scope will be known by the SDK - no need to add it to Enabled as the parameter.

Oh that is correct!

[cijothomas]

Regd. EventName being used for EnabledCheck:

1. It is very important (actually the EventId, the numerical, machine friendly version of EventName that is most important to do ultra fast checks!) for scenarios we are working on in OTel Rust, C++. I don't know if it is something every language/implementation cares about. Given spec does not prohibit an implementation from allowing more parameters, I am totally okay if spec does not mention it, as OTel C++/Rust can offer this as extras.
2. From the Event Oteps, there were mention of scenarios where Loggers can filter based on EventName, route things to different places based on EventName etc. It'd be good to wait to see the progress on Events before we can really say if EventName is a important parameter for the Enabled check.

[reyang]

Not a blocker for this PR