fix: apply filtering EmitLogRecord#4079
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4079 +/- ##
==========================================
+ Coverage 82.01% 82.08% +0.07%
==========================================
Files 385 386 +1
Lines 16030 16131 +101
==========================================
+ Hits 13145 13239 +94
- Misses 2885 2892 +7
🚀 New features to boost your workflow:
|
proost
changed the title
…m/proost/opentelemetry-cpp into fix-apply-filtering-emitlogrecord
dbarker
left a comment
dbarker
left a comment
There was a problem hiding this comment.
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.
…m/proost/opentelemetry-cpp into fix-apply-filtering-emitlogrecord
…y-cpp into fix-apply-filtering-emitlogrecord
| { | ||
| const EventId *event_id_ptr = detail::FindEventIdInArgs(args...); | ||
| #if OPENTELEMETRY_ABI_VERSION_NO >= 2 | ||
| const bool is_enabled = |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thx, 5e67680
how about go through trace based filtering using current option?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I would not add a new logger configuration option just for this. The spec defines the observable Enabled behavior, but the C++ SDK can still implement it efficiently.
For performance, I think this should stay as an internal cache/capability decision: full filtering is needed when trace_based is enabled or when a processor actually implements Enabled filtering. Otherwise the normal enabled path should avoid paying the full virtual/context/processor chain.
One thing to watch is that the cache must stay correct if processors are added after a logger is created.
There was a problem hiding this comment.
OK, I integrate Context And SpanContext both allowed. 8a18dc7
…/opentelemetry-cpp into fix-apply-filtering-emitlogrecord
…/opentelemetry-cpp into fix-apply-filtering-emitlogrecord
…to fix-apply-filtering-emitlogrecord
…to fix-apply-filtering-emitlogrecord
…to fix-apply-filtering-emitlogrecord
…to fix-apply-filtering-emitlogrecord
…/opentelemetry-cpp into fix-apply-filtering-emitlogrecord
…y-cpp into fix-apply-filtering-emitlogrecord
…to fix-apply-filtering-emitlogrecord
…/opentelemetry-cpp into fix-apply-filtering-emitlogrecord
dbarker
left a comment
dbarker
left a comment
There was a problem hiding this comment.
Thanks for the updates. This is looking good. Please see a few minor points below.
| // Logger subclass apply richer filtering (trace-based, processor-level | ||
| // Enabled, custom predicates). | ||
| // | ||
| mutable uint8_t extended_enabled_required_{0}; |
There was a problem hiding this comment.
This is an ABI breaking change. Please guard the new member on ABI v2 along with locations where used.
There was a problem hiding this comment.
Sorry to miss that. you are right. It should be gated.
…to fix-apply-filtering-emitlogrecord
6 participants
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
EmitLogRecordmethod.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.mdupdated for non-trivial changes