-
Notifications
You must be signed in to change notification settings - Fork 438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BUILD] DLL export interface for Metrics #2344
Conversation
Signed-off-by: Harish Shan <[email protected]>
Signed-off-by: Harish Shan <[email protected]>
Signed-off-by: Harish Shan <[email protected]>
Signed-off-by: Harish Shan <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #2344 +/- ##
=======================================
Coverage 87.43% 87.43%
=======================================
Files 199 199
Lines 6030 6030
=======================================
Hits 5272 5272
Misses 758 758 |
Signed-off-by: Harish Shan <[email protected]>
@@ -1,6 +1,12 @@ | |||
# Copyright The OpenTelemetry Authors | |||
# SPDX-License-Identifier: Apache-2.0 | |||
|
|||
if(DEFINED OPENTELEMETRY_BUILD_DLL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a problem of this PR, but I think we should use if(OPENTELEMETRY_BUILD_DLL)
instead of if(DEFINED OPENTELEMETRY_BUILD_DLL)
. And use target_compile_definitions(opentelemetry_api INTERFACE OPENTELEMETRY_BUILD_IMPORT_DLL)
instead of add_definitions(-DOPENTELEMETRY_BUILD_IMPORT_DLL)
, so we can exporting this definition to cmake package.
Signed-off-by: Harish Shan <[email protected]>
Thanks for the contribution. The PR looks good. The current CI is only executing traces example as here - opentelemetry-cpp/ci/do_ci.ps1 Line 77 in e918960
Should we also add the metrics example here, to validate that the example is using the DLL correctly. |
Signed-off-by: Harish Shan <[email protected]>
Hi, I have added both the metrics and logs tests to the cmake.dll.test workflow. Let me know if any other changes are required. Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Good to have @ThomsonTan approval before merging, as he wrote the initial DLL code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution.
@perhapsmaple thanks a lot for working on this! we discovered this today. we have been trying to modify otel-cpp locally to add metrics support for windows dll - we are however having some build issues. Could you please have a look, perhaps you can help us solve it? :) #2202 (comment) |
Fixes #2313
Changes
Add Metrics classes to exported DLL interface.
For significant contributions please make sure you have completed the following items:
CHANGELOG.md
updated for non-trivial changes