Revert Logger.IsEnabled to not require Record

[bogdandrutu]

Resolves https://github.com/open-telemetry/opentelemetry-go/issues/5769

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 84.5%. Comparing base (9339b21) to head (7bb9ec5). Report is 122 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #5770     +/-   ##
=======================================
- Coverage   84.5%   84.5%   -0.1%     
=======================================
  Files        272     272             
  Lines      22759   22753      -6     
=======================================
- Hits       19253   19233     -20     
- Misses      3166    3180     +14     
  Partials     340     340             

see 5 files with indirect coverage changes

[pellared]

How about changing the Enabled to accept a simple struct named e.g. EnabledParams with getters and setters (in order to avoid heap allocations)? The getters should also return a bool if the value was set.

EDIT: However, the possible alternative could be to update the Record getters to do that (return two values instead of one). But this would not allow extending the Emit and Enabled methods' inputs separately.

Can we also rename the method (from Enabled to IsEnabled) in a seperate PR?

[bogdandrutu]

Adding options would decrease the performance (by adding heap allocations) on a path where users expect high-performance.

Reference: https://github.com/open-telemetry/opentelemetry-go/blob/main/log/DESIGN.md#options-as-parameter-to-loggeremit

Sure, but then we must have it extensible, so we need a way to be able to add more params. A different pattern that avoids allocations is to have a struct "Settings/Config" instead, but I followed your patterns.

[pellared]

A different pattern that avoids allocations is to have a struct "Settings/Config" instead [...]

This seems acceptable to me (as noted in https://github.com/open-telemetry/opentelemetry-go/pull/5770#issuecomment-2327444064).

EDIT: At minimum, we need to be able to set the log record severity level. Needed for: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/processors/minsev/minsev.go

[bogdandrutu]

EDIT: At minimum, we need to be able to set the log record severity level. Needed for: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/processors/minsev/minsev.go

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

[MrAlias]

EDIT: At minimum, we need to be able to set the log record severity level. Needed for: https://github.com/open-telemetry/opentelemetry-go-contrib/blob/main/processors/minsev/minsev.go

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

This was removed from the specification based on the feedback of the spec SIG during a meeting: https://github.com/open-telemetry/opentelemetry-specification/pull/4020/commits/19ade6db74d62bd139f3e81f8497342f2da393c4

[pellared]

This was removed from the specification based on the feedback of the spec SIG during a meeting: https://github.com/open-telemetry/opentelemetry-specification/commit/19ade6db74d62bd139f3e81f8497342f2da393c4

@MrAlias, do you know the reason why the following was removed:

Users of a language implementation community can have specific needs when using
this API. Therefore, additional parameters MAY be accepted.

The fact that something can be added in future does not mean that it can accept any parameters right now.

Are you able to find the SIG meeting date so that I can watch the recording?

[MrAlias]

Are you able to find the SIG meeting date so that I can watch the recording?

My best guess would be May 7th.

[bogdandrutu]

The fact that something can be added in future does not mean that it can accept any parameters right now.

Correct, that is why I did this PR.

[pellared]

This was removed from the specification based on the feedback of the spec SIG during a meeting: https://github.com/open-telemetry/opentelemetry-specification/commit/19ade6db74d62bd139f3e81f8497342f2da393c4

From my understanding it was removed mostly because @jack-berg just wanted to decrease the scope of the PR and postpone the discussions around parameters. From transcript (a little cleaned up):

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.

[pellared]

Issue was solved in https://github.com/open-telemetry/opentelemetry-go/pull/5791