fix: apply filtering EmitLogRecord

[proost]

Fixes #2667

Changes

prev: #4011

fix: Note that EmitLogRecord() helpers never honor the Enabled() flag either.

A huge gap exists between API/SDK spec and C++ client for EmitLogRecord method.

1. missing context parameter
2. missing exception parameter
3. support severity filtering in the "SDK".

1 is easy, although ABI level breaking change is inevitable.
2 is quite confusing to me. Is just another attributes or std::exception? When i read data-model spec, It feels like one of attributes.
3 needs decision. Because In SDK level interface, we use "LogRecord" type. But we can't extract severity from "LogRecord" easily. Or do we have to overload "EmitLogRecord" with severity?

So i change very narrower code. Filtering in the severity filtering in the API level and trace based filtering in the sdk level.

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[codecov]

Codecov Report

❌ Patch coverage is 57.57576% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.89%. Comparing base (76f0872) to head (80051a4).

Files with missing lines	Patch %	Lines
sdk/src/logs/logger.cc	3.85%	25 Missing ⚠️
api/include/opentelemetry/logs/logger.h	86.37%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4079      +/-   ##
==========================================
- Coverage   82.01%   81.89%   -0.11%     
==========================================
  Files         385      385              
  Lines       16023    16089      +66     
==========================================
+ Hits        13139    13175      +36     
- Misses       2884     2914      +30     

Files with missing lines	Coverage Δ
...pi/include/opentelemetry/logs/logger_type_traits.h	100.00% <100.00%> (ø)
api/include/opentelemetry/logs/logger.h	75.91% <86.37%> (+7.06%)	⬆️
sdk/src/logs/logger.cc	69.37% <3.85%> (-20.04%)	⬇️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[dbarker]

Thanks for the PR. We definitely need some discussion here (thanks for starting it) on how to align with the spec. As you noted there are a few gaps in how the full Context is handled and Enabled is checked when creating and emitting a log record. Generally it seems there should be a path to explicitly provide the full Context and avoid any implicit calls to get the current context from RuntimeContext.

[dbarker]

The check above to logger_config_.IsEnabled() is not really complete and may be the wrong place to check if the logger is Enabled. The spec defines multiple conditions that can disable the logger.

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/logs/sdk.md#enabled

Enabled MUST return false when either:

• there are no registered LogRecordProcessors.
  Status: Development
• Logger is disabled (LoggerConfig.enabled is false).
  Status: Development
• the provided severity is specified (i.e. not 0) and is less than the configured minimum_severity in the LoggerConfig.
  Status: Development
• trace_based is true in the LoggerConfig and the current context is associated with an unsampled trace.
• all registered LogRecordProcessors implement Enabled, and a call to Enabled on each of them returns false.

The current implementation only checks for the LoggerConfig.enabled flag (skipping the other conditions).

Also creating a log record now (when enabled) will always implicitly get the current context, even if the user has explicitly provided the context (trace id, span id, trace flags) on emit. If a user is able to and provides a full context on emit then getting the current context should be avoided.

This may mean we need to update the Logger API EmitLogRecord API to accept a full context and only get the current context in the emit method if one is not provided.

The Enabled check may then need to live in the EmitLogRecord method and guard creating a log record. If a user decides to call CreateLogRecord directly then perhaps it is their responsibility to call the Enabled method if needed.

[proost]

OK, here is what i understand. We align spec through API level using current "Enabled".
47d1e37

Is this correct?

[dbarker]

Getting the runtime context here duplicates the call in CreateLogRecord. In general if the user provides a full context on emit then all other calls that take a context should use the one explicitly provided and avoid getting the current.

[proost]

#4079 (comment)

[owent]

LGTM, thanks.

[lalitb]

Can we avoid making every enabled log call pay for the full Enabled(...) chain? The disabled and below-min-severity paths stay cheap, but the normal enabled path now pays virtual/context/processor cost before record creation. It may be better to cache whether full filtering is actually needed, and only call the full chain when trace-based filtering or processor-level filtering is configured.

[proost]

Thx, 5e67680

how about go through trace based filtering using current option?