[EXPORTER] Rework OTLP/HTTP and OTLP/GRPC exporter options #2388
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #2388 +/- ##
=======================================
Coverage 87.43% 87.43%
=======================================
Files 199 199
Lines 6028 6028
=======================================
Hits 5270 5270
Misses 758 758 |
marcalff
changed the title
|
Also tested with examples: This proves example_otlp_grpc_metric uses Likewise for logs: The log exporter uses |
owent
reviewed
Oct 31, 2023
exporters/otlp/include/opentelemetry/exporters/otlp/otlp_grpc_exporter_options.h
Show resolved
Hide resolved
|
I've attempted to build this on windows doing the following steps:
open a x64 visual studio command prompt for ninja and cmake configuration at cmd line.
This quickly errors with probably just a missing header output: when i try to build with WITH_STL option set to off it gets quite a bit further in compilation ([360/376]). If i build with it only building WITH_OTLP_HTTP=ON and everything else off, It does compile. I have not tested that dll with an actual program that does anything. But it does build on my machine. output from the first time ninja prints "failed" (WITH_STL=OFF): Let me know if anything i've done to test this is wrong and how i should redo it. Feel free to ping me for anything Windows DLL related. Building/Code/Testing changes related. |
|
After the fix from @perhapsmaple and the latest changes i've attempted to build it again. It does not fully build but is close. It fails on the last linking stage of the DLL from what i can se. This symbol that is missing is included in the opentelemetry_cpp.src file at line 96. The addition to the command from last time i was building is the option WITH_OTLP_GRPC set to ON. I've attempted to build this WITH_STL=OFF and WITH_STL=ON and there was no difference. commands:
output: Let me know if my building is wrong in any way or if i should enable/disable more options. Thanks for working on this. |
|
The function definition for OtlpGrpcLogRecordExporterFactory::Create has changed from using |
This reverts commit 544707e.
As far as I know, the remaining items to make this PR complete and also link with windows DLL is to export:
plus the signature change for the GRPC log exporter factory::Create() method. I was hoping to see build failures in CI logs for existing examples or tests, then adjust exports accordingly, but this part is not covered yet in CI. This makes it incredibly difficult to investigate for me (and I shall say, frustrating), as I am not running an a windows platform locally: I can not remote debug a build failure for a different platform that is not even reported ;-( @perhapsmaple If you could investigate this part, it would be awesome, and also very appreciated. Thanks. And thanks @Dangeroustuber for testing. |
|
Hi @marcalff, I have a question regarding one of the examples. In example_otlp_http_log, we directly use I thought the One more thing, I cloned marcalff:fix_exporter_option_1845 and updated the symbols to fix all the linker errors. Is there a better way to update the changes than copy pasting the new file here. I'm a little new to this but I can't seem to submit a PR directly from that. Any help would be appreciated. |
I agree. I believe that |
Good point. To clarify, the application code no longer has to call the However, for whatever reason, the application code still may use some of this helpers functions (as seen in this example), so to be on the safe side we still have to export all these helpers.
In theory you could clone off my own fork, file a PR against it, which I can then merge to my own branch in this PR ... but I would probably need to setup my own branch to accept PR, etc, to make this possible. That is a lot of work for both of us, just to adjust one file, so in practice please paste the diff here in a comment, and I will adjust the code accordingly. I will add credits in the PR comments as well. Thanks. |
@marcalff Here is the git diff. This should fix all the linker errors. Regarding the 32-bit export definitions, I'm facing some msbuild errors on my windows VM while trying to build a 32-bit DLL with cmake, I'll add those definitions once I figure out how to fix it. |
owent
approved these changes
Nov 12, 2023
ThomsonTan
approved these changes
Nov 13, 2023
|
It looks like adding the default ctors has broken a quite convenient way to initialize OtlpGrpcExporterOptions via C++20 designated initializers: This is a part of our code that was broken by this change: Looks legit to me. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1845
Contributes to #2202
Contributes to #1429
Changes
This fix refactored the OTLP/HTTP and OTLP/GRPC exporter options.
OTLP/GRPC exporters uses the wrong environment variables
Before the fix, the class hierarchy was:
After the fix, it is changed to:
This resolved issue #1845
OTLP/GRPC exporters causes a lot of symbols to be visible
Reading the default configuration from environment variables,
with helpers such as
GetOtlpDefaultGrpcEndpoint(),was done in code inlined in the header file.
This causes various
GetOtlpDefaultGrpcEndpoint()helpers to leak into the application at build time,causing issues when building a DLL on Windows when
GetOtlpXXX()helpers are not exported.This is fixed by declaring a constructor for options structs,
and moving all the default initialization in the constructor implementation,
located in a
*.ccfile, in the opentelemetry-cpp libraries.Now that the application code no longer must sees
GetOtlpXXX()helpers, the dependencies are simpler.Note that helper functions
GetOtlpXXX()are still visible and exported, for applications that uses them directly.This contributes to fix issues #2202 and #1429
OTLP/HTTP exporters causes a lot of symbols to be visible
Likewise for the OTLP/HTTP exporter.
This contributes to fix issues #2202 and #1429
Added missing OTLP/GRPC unit tests for the Metrics exporter.
The unit tests added only check the configuration part.
Adjust list of symbols exported on Windows, when building a DLL.
File
ext/src/dll/opentelemetry_cpp.srcis adjusted to export the new options structs constructors and destructors.Thanks to @perhapsmaple for contributing this change.
Follow up work
The following is not part of the PR, to be done in separate fixes:
OtlpHttpClientOptionsFor significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes