Move implementation from SDK header files to SDK cc#3887
Move implementation from SDK header files to SDK cc#3887perhapsmaple wants to merge 7 commits intoopen-telemetry:mainfrom
Conversation
Signed-off-by: Harish <140232061+perhapsmaple@users.noreply.github.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3887 +/- ##
==========================================
+ Coverage 90.06% 90.09% +0.03%
==========================================
Files 226 229 +3
Lines 7223 7253 +30
==========================================
+ Hits 6505 6534 +29
- Misses 718 719 +1
🚀 New features to boost your workflow:
|
Signed-off-by: Harish <140232061+perhapsmaple@users.noreply.github.com>
Signed-off-by: Harish <140232061+perhapsmaple@users.noreply.github.com>
Signed-off-by: Harish <140232061+perhapsmaple@users.noreply.github.com>
Signed-off-by: Harish <140232061+perhapsmaple@users.noreply.github.com>
marcalff
left a comment
There was a problem hiding this comment.
Thanks for the fix.
This has a good size already, so please stabilize this PR to make it ready for review and merge. More files can be changed in a different PR, if so inclined.
To fix:
- IWYU build failures
- look for "Warning: include-what-you-use reported diagnostics:" in the build logs, in section iwyu_tool
- Resolve merge conflicts
- clang-format
Signed-off-by: Harish <140232061+perhapsmaple@users.noreply.github.com>
| */ | ||
| static const opentelemetry::common::KeyValueIterableView< | ||
| std::array<std::pair<std::string, int32_t>, 0>> & | ||
| GetEmptyAttributes() noexcept |
There was a problem hiding this comment.
GetEmptyAttributes() was previously a static free function (header-only, internal linkage). It's now an externally-linked symbol requiring linkage against opentelemetry_common. While this shouldn't be an issue in practice, someone could theoretically be including this header standalone without linking the SDK - for their own utility purposes - and that would now fail with an undefined symbol error. Please add a CHANGELOG entry noting this change.
lalitb
left a comment
There was a problem hiding this comment.
Thanks for the PR. LGTM once the CI and merge conflict are resolved.
| * @return the attributes for this event | ||
| */ | ||
| const std::unordered_map<std::string, opentelemetry::sdk::common::OwnedAttributeValue> & | ||
| GetAttributes() const noexcept |
There was a problem hiding this comment.
Move this and SpanDataLink::GetAttributes to .cc for consistency, since SpanData::GetAttributes has been moved to .cc.
Contributes to #1429
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