Specs inconsistent Enabled API for Logger

[bogdandrutu]

Description

Current definition of the Logger.Enabled says in the specs:

To help users avoid performing computationally expensive operations when
generating a LogRecord, a Logger SHOULD provide this Enabled API.

There are currently no required parameters for this API. Parameters can be
added in the future, therefore, the API MUST be structured in a way for
parameters to be added.

See the current implementation here. This means that users must construct the Record before checking if it is enabled, at this point this API is useless since I can call directly emit which can drop the Record. This API must not require the record to be passed.

Expected behavior

It should not require to construct a Record to check if the logger is enabled.

[MrAlias]

You can construct a partial record and check that though in the referenced implementation. If you only want to check severity, you only set the severity of the record passed. This means performing the actual complex work of determining other fields may be avoided.

[bogdandrutu]

You can construct a partial record and check that though in the referenced implementation. If you only want to check severity, you only set the severity of the record passed. This means performing the actual complex work of determining other fields may be avoided.

Even if that is an use-case, I think that should be done via "options" not required. I would imagine having an option "WithSeverity" for example to do this.

[bogdandrutu]

@MrAlias Another inconsistency is that the API was not extensible, per the specs requirement.

[bogdandrutu]

If you only want to check severity, you only set the severity of the record passed. This means performing the actual complex work of determining other fields may be avoided.

This is also a problem because you may lose events here. Think if one of the Filter processor accepts only records with body == "foo". This processor will return that the logger is disabled if I check for logger.Enabled(ctx, log.Recors{Severity: INFO} because the body is empty. So I cannot actually use with partial records, unless I am missing something.

[pellared]

Think if one of the Filter processor accepts only records with body == "foo".

The processor should first check if body is not empty using https://pkg.go.dev/go.opentelemetry.io/otel/log#Value.Empty before doing the actual filtering to check if any value was assigned.

However, there may be actually a problem if the body is actually empty and we would like to e.g. filter out log records with empty bodies. I think, we were discussing it with @MrAlias, but I cannot find the conclusion anywhere. There was indeed an idea to create a separate struct for Enabled input that would be able to distinguish "unset fields" vs "fields set to empty". Maybe we thought that it is YAGNI?

[bogdandrutu]

However, there may be actually a problem if the body is actually empty and we would like to e.g. filter out log records with empty bodies.

This opens another problem, because that may pass records that should not pass the filter processor.

There was indeed an idea to create a separate struct for Enabled input that would be able to distinguish "unset fields" vs "fields set to empty". Maybe we thought that it is YAGNI?

Ideally this should be optional to pass, and ideal not Record. Hence the API is not consistent with specs.

[pellared]

This opens another problem, because that may pass records that should not pass the filter processor.

I also want to call out that the same problem exists for Emit because the Record also does not allow distinguishing unset fields from fields explicitly set to a "zero value".

[pellared]

Based on convo in #5770 (comment).

I would prefer this in specs before adding support to be honest.

I agree. What do you think of something like the following?

The API SHOULD accept the same set of parameters as [Emit a LogRecord](#emit-a-logrecord):

- [Timestamp](./data-model.md#field-timestamp)
- [Observed Timestamp](./data-model.md#field-observedtimestamp)
- The [Context](../context/README.md) associated with the `LogRecord`. The API
  MAY implicitly use the current Context as a default
  behavior.
- [Severity Number](./data-model.md#field-severitynumber)
- [Severity Text](./data-model.md#field-severitytext)
- [Body](./data-model.md#field-body)
- [Attributes](./data-model.md#field-attributes)

Other parameters that are not accepted by [Emit a LogRecord](#emit-a-logrecord)
can be added in the future, therefore, the API MUST be
structured in a way for parameters to be added.

All parameters MUST be optional.

It MUST be possible to distinguish between
an unset parameter value from a parameter value set explicitly to
a default value of given type.

@MrAlias, thoughts?

[pellared]

Side note: Whatever we decide. I think we should have a section in https://github.com/open-telemetry/opentelemetry-go/blob/main/log/DESIGN.md for Logger.Enabled that documents the design decisions.

Reference: #5071 (comment)

[pellared]

Based on the discussion during SIG meeting I plan to

1. Make a PR that would change Enabled to accept a newly created EnabledParam type. The type would accept setting the severity level and getting its value together with a bool telling whether it was actually set (by the bridge) or not.
2. Make a PR in spec defining the Enabled parameters.

[pellared]

I created a PoC (Spike) here: #5791
I would welcome some initial review. If this seems good then I will probably start with a PR for DESIGN.md describing the design decisions.