Build OpenTelemetry C++ SDK and exporter into DLL

[ThomsonTan]

Fixes #1105

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[lalitb]

Thanks @ThomsonTan . Just to be clear, is this going to resolve the #1105 ?

@meastp, @owent - It would be really helpful whenever you have time to review this.

[codecov]

Codecov Report

Merging #1932 (bca6227) into main (6f0a30e) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1932      +/-   ##
==========================================
+ Coverage   87.12%   87.13%   +0.01%     
==========================================
  Files         165      166       +1     
  Lines        4596     4597       +1     
==========================================
+ Hits         4004     4005       +1     
  Misses        592      592              

Impacted Files	Coverage Δ
api/include/opentelemetry/baggage/baggage.h	97.35% <ø> (ø)
...ntelemetry/context/propagation/global_propagator.h	100.00% <ø> (ø)
...pi/include/opentelemetry/context/runtime_context.h	97.60% <ø> (ø)
api/include/opentelemetry/trace/noop.h	93.55% <ø> (ø)
api/include/opentelemetry/trace/provider.h	100.00% <ø> (ø)
api/include/opentelemetry/trace/trace_state.h	97.60% <ø> (ø)
api/include/opentelemetry/trace/tracer_provider.h	100.00% <ø> (ø)
sdk/include/opentelemetry/sdk/trace/exporter.h	100.00% <ø> (ø)
sdk/include/opentelemetry/sdk/trace/processor.h	100.00% <ø> (ø)
...k/include/opentelemetry/sdk/trace/tracer_context.h	100.00% <100.00%> (ø)

[ThomsonTan]

Yes, it should resolve the issues in #1105. I still need some minor change on this draft, but feel free to take a look as the major change has been verified.

I plan to add some document on exporting C++ inline member function to DLL.

Thanks @ThomsonTan . Just to be clear, is this going to resolve the #1105 ?

@meastp, @owent - It would be really helpful whenever you have time to review this.

[marcalff]

In:

  class OPENTELEMETRY_API TracerProviderFactory

It feels strange to see "API" in the name, for a class from the SDK.

IMPORT / EXPORT is orthogonal to api/sdk/etc

Rename to OPENTELEMETRY_EXPORT, which will apply to any api / sdk / exporter classes ?

[ThomsonTan]

In:

  class OPENTELEMETRY_API TracerProviderFactory

It feels strange to see "API" in the name, for a class from the SDK.

IMPORT / EXPORT is orthogonal to api/sdk/etc

Rename to OPENTELEMETRY_EXPORT, which will apply to any api / sdk / exporter classes ?

I actually named the macro as like OPENTELEMETRY_EXPORT_IMPORT, but then changed to OPENTELEMETRY_API because all the classes/functions marked as by this implies that they are exported to user to reference/invoke. So OPENTELEMETRY_API here all the OpenTelemetry APIs and a few selected SDK method we want to expose to user to initialize SDK. But we can discuss the name here.

[lalitb]

In:

  class OPENTELEMETRY_API TracerProviderFactory

It feels strange to see "API" in the name, for a class from the SDK.
IMPORT / EXPORT is orthogonal to api/sdk/etc
Rename to OPENTELEMETRY_EXPORT, which will apply to any api / sdk / exporter classes ?

I actually named the macro as like OPENTELEMETRY_EXPORT_IMPORT, but then changed to OPENTELEMETRY_API because all the classes/functions marked as by this implies that they are exported to user to reference/invoke. So OPENTELEMETRY_API here all the OpenTelemetry APIs and a few selected SDK method we want to expose to user to initialize SDK. But we can discuss the name here.

Don't have strong opinion on naming, but OPENTELEMETRY_EXTERN or OPENTELEMETRY_WIN_EXTERN can be considered too, if we want to avoid API and EXPORT keywords :)

[ThomsonTan]

Looked at some Chromium code,

In:

  class OPENTELEMETRY_API TracerProviderFactory

It feels strange to see "API" in the name, for a class from the SDK.
IMPORT / EXPORT is orthogonal to api/sdk/etc
Rename to OPENTELEMETRY_EXPORT, which will apply to any api / sdk / exporter classes ?

I actually named the macro as like OPENTELEMETRY_EXPORT_IMPORT, but then changed to OPENTELEMETRY_API because all the classes/functions marked as by this implies that they are exported to user to reference/invoke. So OPENTELEMETRY_API here all the OpenTelemetry APIs and a few selected SDK method we want to expose to user to initialize SDK. But we can discuss the name here.

Don't have strong opinion on naming, but OPENTELEMETRY_EXTERN or OPENTELEMETRY_WIN_EXTERN can be considered too, if we want to avoid API and EXPORT keywords :)

Looked some Chromium code and pattern, seems naming it as *_EXPORT makes sense as suggested by @marcalff. See https://chromium.googlesource.com/chromium/src.git/+/master/base/base_export.h

[meastp]

@latlib I'm on leave, so haven't looked at the changes in detail, but this is fantastic. Great work @ThomsonTan! :)

When I did my "fork" I had problems with some of the examples/tests. I copied the shared dlls manually, which made it a bit cumbersome, but I think cmake has some function to fix this.
Anyway (as you know, of course), it is important to check the examples, otlp with both http and grpc (I had some threading trouble, but hopefully this PR/newer library versions have fixed this. If the problems i encountered is still present, they will be visible just by running tests and/or examples, so I wouldn't worry much about it. :)

Again, this makes me very happy - thanks! well done :)

[marcalff]

(struggling with github UI, restart review to remove requested changes flag)

[lalitb]

Having a single DDL for opentelemetry-cpp and all the exporters is still an area of discussion:

• should not be the long term goal in my opinion,
• ok to have this single DDL as an intermediate step, assuming we document this is experimental and subject to change.

Agreed, with single DLL for opentelemetry-cpp, there may be further requests for single shared/static library for other platforms too :). As of now, building OpenTelemetry C++ as shared target(s) for Windows user is something missing, and this looks to be a promising first step towards it. Also building multiple DDL targets ( specifically for API ) would require us to either move away from header-only API support, or create source-api from header during build - and this needs to be discussed further.
We can discuss this in the community meeting, my vote would be to provide this feature as experimental, and clearly document the constraints.

As @marc is not able to remove the flag.

[lalitb]

(struggling with github UI, restart review to remove requested changes flag)

Have removed the flag, hope it works.

[ThomsonTan]

@owent could you please help review this PR when you have time? Thanks.

[marcalff]

This is buried in comments, so repeating here to avoid missing it.

Please consider this suggestion:

The choice between import and export is driven by which file is compiled, not by the user code.

In CMakeList.txt:

if(DEFINED OPENTELEMETRY_BUILD_DLL)
  if(NOT MSVC)
    message(WARNING "Build DLL is supposed to work with MSVC!")
  endif()
  if(WITH_STL)
    message(
      WARNING "Build DLL with C++ STL could cause binary incompatibility!")
  endif()
  add_definitions(-DOPENTELEMETRY_BUILD_DLL) // <-------------------------
endif()

In macro.h:

#if defined(_MSC_VER) && defined(OPENTELEMETRY_BUILD_DLL)

#ifdef OPENTELEMETRY_BUILD_EXPORT_DLL // <-------------------------------------
#  define OPENTELEMETRY_EXPORT __declspec(dllexport)
#else
#  define OPENTELEMETRY_EXPORT __declspec(dllimport)
#endif

#else

//
// build OpenTelemetry as static library or not on Windows.
//
#  define OPENTELEMETRY_EXPORT

#endif

And then, when compiling every file that is part of the DLL:

File sdk/src/trace/CMakeLists.txt

add_definitions(-DOPENTELEMETRY_BUILD_EXPORT_DLL) // <------------------------------

This totally avoids user to add OPENTELEMETRY_BUILD_IMPORT_DLL.

[ThomsonTan]

Currently there are 3 macros, OPENTELEMETRY_BUILD_DLL which is a CMake option, OPENTELEMETRY_BUILD_EXPORT_DLL and OPENTELEMETRY_BUILD_IMPORT_DLL which are preprocess macros for C++ source files.

The suggestion seems about merging the CMake and one of the C++ macro definition which indeed can eliminate one def. So the point is that OPENTELEMETRY_BUILD_DLL alone implies importing OpenTelemetry DLL, and OPENTELEMETRY_BUILD_DLL with OPENTELEMETRY_BUILD_EXPORT_DLL implies building SDK itself as DLL.

My concern is that this extra implicit assumption makes the intention less clear to the user. For example, ask the user to define C++ macro OPENTELEMETRY_BUILD_DLL could cause confusion as the actual behavior is importing OpenTelemetry from the published DLL/LIB (nothing is forced to be built into DLL in this case). In the other way, changing OPENTELEMETRY_BUILD_DLL to OPENTELEMETRY_BUILD_IMPORT_DLL could cause similar confusion to SDK developers.

This totally avoids user to add OPENTELEMETRY_BUILD_IMPORT_DLL.

[owent]

Just curious, is it better to export all classes and functions when building a dll?

[ThomsonTan]

Just curious, is it better to export all classes and functions when building a dll?

@owent what does it mean for exporting all classes? Even the classes internal to API/SDK/Exporters? If so, this could make the output DLL very fragile as any change on the internal classes could break the binary to be loaded to the app which linked to the older DLL, even there is not API change in this repo.

[owent]

Just curious, is it better to export all classes and functions when building a dll?

@owent what does it mean for exporting all classes? Event the classes internal to API/SDK/Exporters? If so, this could make the output DLL very fragile as any change on the internal classes could break the binary to be loaded to the app which linked to the older DLL, even there is not API change in this repo.

Thanks for your explanation.I have another question, we install all headers when we run cmake --install now.In my understanding, is it better to select a subset of headers to install(all exported classes and their dependencies). Maybe we can also improve it in another PR.

[owent]

LGTM

[ThomsonTan]

is it better to select a subset of headers to install(all exported classes and their dependencies)

Filed #1980 to track this.