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".
4. We currently handle ObservedTimestamp and Timestamp using same type. Spec tells both optional parameter how to divide which one is ObservedTimestamp and other one is Timestamp?

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?
4 needs a design.

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 91.45299% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.07%. Comparing base (88d088d) to head (d62c8e4).

Files with missing lines	Patch %	Lines
sdk/src/logs/logger.cc	80.65%	6 Missing ⚠️
api/include/opentelemetry/logs/logger.h	94.45%	2 Missing ⚠️
...pi/include/opentelemetry/logs/logger_type_traits.h	95.24%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4079      +/-   ##
==========================================
+ Coverage   81.99%   82.07%   +0.08%     
==========================================
  Files         385      386       +1     
  Lines       16023    16121      +98     
==========================================
+ Hits        13137    13230      +93     
- Misses       2886     2891       +5     

Files with missing lines	Coverage Δ
...pentelemetry/sdk/logs/batch_log_record_processor.h	100.00% <100.00%> (ø)
sdk/include/opentelemetry/sdk/logs/processor.h	100.00% <100.00%> (ø)
...entelemetry/sdk/logs/simple_log_record_processor.h	100.00% <100.00%> (ø)
sdk/src/logs/multi_log_record_processor.cc	93.68% <100.00%> (+0.43%)	⬆️
api/include/opentelemetry/logs/logger.h	80.42% <94.45%> (+11.56%)	⬆️
...pi/include/opentelemetry/logs/logger_type_traits.h	97.62% <95.24%> (-2.38%)	⬇️
sdk/src/logs/logger.cc	89.70% <80.65%> (+0.28%)	⬆️

... and 3 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]

Thanks. Looks reasonable. There still seems to be a gap in handling user provided Context in parts (as SpanContext or TraceID + SpanID + TraceFlags):

1. User calls EmitLogRecord(args...) with SpanContext -> Enabled called with Current Runtime Context instead of a Context with the provided SpanContext?
2. User calls EmitLogRecord(args...) with TraceID, SpanID, and TraceFlags -> Enabled is checked against Current Runtime Context instead of a Context from the provided trace parts?

[proost]

d4578ad

We should cover this too. Thank you.

[dbarker]

Thanks for the update. This change appears to result in the right behavior. If a user provides trace args independently as SpanContext or TraceID+SpanID+TraceFlags then those args are used to determine if the record should be created and to set the record's traceid, spanid, and traceflags.

The downside for current users is the extra dynamic memory allocations to reconstruct a full Context from the args (allocating a DefaultSpan just to later get the trace data from it). We should avoid this cost for those users if possible.

One option: Instead of constructing a full Context object when a user provides SpanContext or TraceID+SpanID+TraceFlags we pass just a SpanContext (construct if needed) through to the sdk methods. We could do this with a nostd::variant<SpanContext, context::Context> similar to StartSpanOptions or define new struct. That would require updating the ABIv2 interface for CreateLogRecord, EnabledImplementation, and IsAllowedByTraceBasedFiltering to take this object referencing either a full Context or SpanContext.

What do you think? @lalitb any thoughts on this approach?

[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?

[lalitb]

Thanks, this helps with the trace-based case, but I think the original concern is only partially addressed. We still need the cache to account for processor-level filtering too, otherwise the normal non-trace-based path can skip processor Enabled(...) checks.

[proost]

Ok, now i understand your intention.
dc306d5

Later, i will add logger configuration.. this option might be needed in the configuration. But this is not in the spec. is ok?