[SDK] support for the new OTel log API #2123
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2123 +/- ##
=======================================
Coverage 87.19% 87.19%
=======================================
Files 166 166
Lines 4784 4784
=======================================
Hits 4171 4171
Misses 613 613 |
| }; | ||
|
|
||
| template <> | ||
| struct LogRecordSetterTrait<int64_t> |
There was a problem hiding this comment.
Is it conflicted with Body?Log body can also be converted from int64_t implicitly.
There was a problem hiding this comment.
Yes, thanks for the catch. Seems the template part of filling logging record requires each parameter to be in different type, and the any type on body make it conflict with other parameters. Do you have any suggestion for this problem?
There was a problem hiding this comment.
I have no idea for this problem by now, and I only avoided to specialize templates for these types before.
Maybe we can add a unit test to check the mistake.
There was a problem hiding this comment.
I removed the passing of int64_t to SDK, instead, a temporary EventId struct will be constructed which should fix the issue.
For unittest, do you mean to cover all the variant types of body field?
There was a problem hiding this comment.
Yes, use EmitLogRecord to cover all the variant types of body field.
Maybe we can also add it in another PR later.
There was a problem hiding this comment.
FYI, I added an integration test to cover if the EventId passed to the user-facing API is passed correctly to the SDK.
| * @param id the event Id to set | ||
| * @param name the event name to set | ||
| */ | ||
| void SetEventId(int64_t /* id */, nostd::string_view /* name */) noexcept override |
There was a problem hiding this comment.
Sorry but did I miss anything? I can find any specification about event id in OTLP, should we add it just like semantic convention for event attributes?
There was a problem hiding this comment.
Yes, the idea is adding it as attribute to OTLP recordable, mark it as TODO for now.
|
@owent please let me know if there is any comments I missed. |
marcalff
changed the title
Fixes #2122
Changes
Please provide a brief description of the changes here.
For significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes