search

open-telemetry / opentelemetry-cpp

The OpenTelemetry C++ Client
https://opentelemetry.io/
Apache License 2.0
839 stars 401 forks source link

Support building DLL for windows #1105

Closed lalitb closed 1 year ago

lalitb commented 2 years ago

opentelemetry-cpp uses bazel and cmake to build the required targets. These targets are by default created as static libraries. To create shared libraries / DLLs, we can just add -DBUILD_SHARED_LIBS=ON to cmake, and linkstatic=False flag in cc_library/cc_binary rule in bazel. This works fine for building shared libraries(.so) on Unix-like systems but doesn't work by default for DLL on Windows. This needs code changes to define export/import declarations for all exporter API and classes. As each DLL on Windows will have its own heap for memory usage and symbols.

More discussions from the slack channel here (Need to be part of cloud-native.slack.com Org to view the discussions ): https://cloud-native.slack.com/archives/C01N3AT62SJ/p1626285244072700 https://cloud-native.slack.com/archives/C01N3AT62SJ/p1637665049041200

While there is no immediate plan to support this requirement, unless there is enough traction on this issue, and contributors are ready to add the support.

lalitb commented 2 years ago

Tagging @meastp @owent FYI, and if they would like to add more details here.

owent commented 2 years ago

We can use codes like these to export/import APIs of some libraries.

// ================ import/export: for compilers ================
#if defined(__GNUC__) && !defined(__ibmxl__)
//  GNU C++/Clang
//
// Dynamic shared object (DSO) and dynamic-link library (DLL) support
//
#  if __GNUC__ >= 4
#    if defined(_WIN32) || defined(__WIN32__) || defined(WIN32) || defined(__CYGWIN__)
// All Win32 development environments, including 64-bit Windows and MinGW, define
// _WIN32 or one of its variant spellings. Note that Cygwin is a POSIX environment,
// so does not define _WIN32 or its variants.
#      ifndef OPENTELEMETRY_SYMBOL_EXPORT
#        define OPENTELEMETRY_SYMBOL_EXPORT __attribute__((__dllexport__))
#      endif
#      ifndef OPENTELEMETRY_SYMBOL_IMPORT
#        define OPENTELEMETRY_SYMBOL_IMPORT __attribute__((__dllimport__))
#      endif
#    else
#      ifndef OPENTELEMETRY_SYMBOL_EXPORT
#        define OPENTELEMETRY_SYMBOL_EXPORT __attribute__((visibility("default")))
#      endif
#      ifndef OPENTELEMETRY_SYMBOL_IMPORT
#        define OPENTELEMETRY_SYMBOL_IMPORT __attribute__((visibility("default")))
#      endif
#      ifndef OPENTELEMETRY_SYMBOL_VISIBLE
#        define OPENTELEMETRY_SYMBOL_VISIBLE __attribute__((visibility("default")))
#      endif
#      ifndef OPENTELEMETRY_SYMBOL_LOCAL
#        define OPENTELEMETRY_SYMBOL_LOCAL __attribute__((visibility("hidden")))
#      endif
#    endif
#  else
// config/platform/win32.hpp will define OPENTELEMETRY_SYMBOL_EXPORT, etc., unless already defined
#    ifndef OPENTELEMETRY_SYMBOL_EXPORT
#      define OPENTELEMETRY_SYMBOL_EXPORT
#    endif
#    ifndef OPENTELEMETRY_SYMBOL_IMPORT
#      define OPENTELEMETRY_SYMBOL_IMPORT
#    endif
#    ifndef OPENTELEMETRY_SYMBOL_VISIBLE
#      define OPENTELEMETRY_SYMBOL_VISIBLE
#    endif
#    ifndef OPENTELEMETRY_SYMBOL_LOCAL
#      define OPENTELEMETRY_SYMBOL_LOCAL
#    endif
#  endif
#elif defined(_MSC_VER)
//  Microsoft Visual C++
//
//  Must remain the last #elif since some other vendors (Metrowerks, for
//  example) also #define _MSC_VER
#else
#endif
// ---------------- import/export: for compilers ----------------

// ================ import/export: for platform ================
//  Default defines for OPENTELEMETRY_SYMBOL_EXPORT and OPENTELEMETRY_SYMBOL_IMPORT
//  If a compiler doesn't support __declspec(dllexport)/__declspec(dllimport),
//  its file must define OPENTELEMETRY_SYMBOL_EXPORT and OPENTELEMETRY_SYMBOL_IMPORT
#if !defined(OPENTELEMETRY_SYMBOL_EXPORT) && (defined(_WIN32) || defined(__WIN32__) || defined(WIN32) || defined(__CYGWIN__))

#  ifndef OPENTELEMETRY_SYMBOL_EXPORT
#    define OPENTELEMETRY_SYMBOL_EXPORT __declspec(dllexport)
#  endif
#  ifndef OPENTELEMETRY_SYMBOL_IMPORT
#    define OPENTELEMETRY_SYMBOL_IMPORT __declspec(dllimport)
#  endif
#endif
// ---------------- import/export: for platform ----------------

#ifndef OPENTELEMETRY_SYMBOL_EXPORT
#  define OPENTELEMETRY_SYMBOL_EXPORT
#endif
#ifndef OPENTELEMETRY_SYMBOL_IMPORT
#  define OPENTELEMETRY_SYMBOL_IMPORT
#endif
#ifndef OPENTELEMETRY_SYMBOL_VISIBLE
#  define OPENTELEMETRY_SYMBOL_VISIBLE
#endif
#ifndef OPENTELEMETRY_SYMBOL_LOCAL
#  define OPENTELEMETRY_SYMBOL_LOCAL
#endif
// ---------------- import/export ----------------

And then, opentelemetry_api for example, declare macros like these:

#define OPENTELEMETRY_API_API_NATIVE // private definition for both static and dynamic library.
#define OPENTELEMETRY_API_API_DLL // public definition for dynamic library only.

// And then declare these.
#if defined(OPENTELEMETRY_API_API_NATIVE) && OPENTELEMETRY_API_API_NATIVE
#  if defined(OPENTELEMETRY_API_API_DLL) && OPENTELEMETRY_API_API_DLL
#    define OPENTELEMETRY_API_API OPENTELEMETRY_SYMBOL_EXPORT
#  else
#    define OPENTELEMETRY_API_API
#  endif
#else
#  if defined(OPENTELEMETRY_API_API_DLL) && OPENTELEMETRY_API_API_DLL
#    define OPENTELEMETRY_API_API OPENTELEMETRY_SYMBOL_IMPORT
#  else
#    define OPENTELEMETRY_API_API
#  endif
#endif
#define OPENTELEMETRY_API_API_HEAD_ONLY OPENTELEMETRY_SYMBOL_VISIBLE

At last, we should add OPENTELEMETRY_API_API to all exported functions and add OPENTELEMETRY_API_API_HEAD_ONLY to all exported template classes and functions like these(we can not export normal class because when built with -fvisibility=hidden the exporting rules for vtable are different in MSVC and gcc/clang):

OPENTELEMETRY_BEGIN_NAMESPACE
namespace trace
{

class Tracer
{
public:
  OPENTELEMETRY_API_API virtual ~Tracer();

  OPENTELEMETRY_API_API  nostd::shared_ptr<Span> StartSpan(nostd::string_view name,
                                    const StartSpanOptions &options = {}) noexcept;

  template <class T,
            nostd::enable_if_t<common::detail::is_key_value_iterable<T>::value> * = nullptr>
  OPENTELEMETRY_API_API_HEAD_ONLY nostd::shared_ptr<Span> StartSpan(nostd::string_view name,
                                    const T &attributes,
                                    const StartSpanOptions &options = {}) noexcept
  {
    return this->StartSpan(name, attributes, {}, options);
  }
};

There is still a problem if we change all declaration of exported APIs above. We can not declare global or static variables in header files, because all modules(exe/dll) will generate their own "global" instance. But it's widely used in opentelemetry_api now. Moving all global and static variables into source files will change opentelemetry_api to not a header only target.

Should we need more discusstion about this?

meastp commented 2 years ago

@owent Thank you for the explanation. I'm currently building otel-cpp locally and I'm using the examples/simple (with foo_library as a separate dll) to test this. I have added dllexport/dllimport, and now need to modify opentelemetry_api and move static variables into source files. I do not think there are a lot of static variables that need extraction, but I'll test and see. I did notice a number of static const constants, but I don't think they require extraction to source files.

As I'm really interested in otel-cpp being useful on Windows, where dynamic dlls are common, I'll try to contribute towards this being supported. I do hope that support for dynamic dlls on windows aligns with otel aspiring to be the de facto observability standard across languages/platforms. :)

meastp commented 2 years ago

Ok, it looks like I got it working with the examples/simple example, and the traceid and parentspanid is correct across simple_example.exe and foo_library.dll (I did start a span before calling foo_library() and ended it right after the call finished). I'm using the ostream_exporter to verify this.

My modifications were only to extract all API methods using static variables that seemed like they might be relevant (i.e. not constant values) to corresponding source files in a src directory (like it was done in the sdk, except I created a single opentelemetry_api.dll):

And I modified CMakeLists.txt to include these files, of course. The conversion was mechanical and straight forward, I think.

I do understand the concern since otel API component would have to be a source library to work (and not header-only). I'm hoping you would welcome contributions to make otel-cpp work with windows shared libraries - having to fork this library to make it work on windows would be a tragedy (at least to me :) ).

So is for example something like what is described by Steve Ire (a CMake maintainer) possibility to have-it-all, even though it does complicate things a bit?

owent commented 2 years ago

Nice try. Just moving global and static variables into source can solve the problem of data sharing.But each dll/exe that use api will still have their copy of codes, it maybe lead to problem if we use the function address(pointer of function or member function) and RTTI(the instance of typeinfo of the same class may be different in different dll/exe) to do some thing.Also It's difficult to maintain the compatibility if there are a lot submodules using otel-cpp and we will link them together finally(Specially some libraries may be published by headers and prebuilt files, not source).So to my understand, it's better to export all public functions and then each function implemention will only have one copy in our dlls.

At the same time, I think we can create a test which creating and setting a global trace provider in one dll, and then use it to create a span in another.If it works well we can do the same modifications to sdk, all the extensions and all exporters.

BTW: I suggest to add -fvisibility=hidden when using gcc or clang to keep the same symbol visibility on Windows and unix like system(linux, macos, ios and etc).

meastp commented 2 years ago

Right, so a better solution to cover all (theoretical?) corner cases is to move the implementation into source files (except templates, of course). I would be happy to do this work, but since otel-cpp has a requirement of header-only, I think we need a solution where one can choose between header-only, static or shared (look at the article I linked to earlier). I want to contribute, but I'm waiting for direction and a decision on how to proceed.

If I understand correctly, that test is exactly what I did: moved foo_library in examples/simple to its own shared library foo_library.dll (used ontly opentelemetry API here) which I used in simple_example.exe (where I initialised the trace provider and the ostream exporter). I created a span that started before calling foo_library() and ended after the call finished. Traceid and parentid seemed correct across the boundaries.

For CMake, there is a handy utility function GenerateExportHeader that helps taking care of generating macros and visibility attributes. I'm not sure if this is useful, as it won't help when building with Bazel.

owent commented 2 years ago

Yes, I also think GenerateExportHeader module is a good replacement of export declaration above, it can generate codes platform by platform and so it's simpler than write codes to adapt all platform.And the test is exactly what I mean before.Maybe the only thing left is the decision on how to proceed. Maybe @lalitb can provide more information about it.

meastp commented 2 years ago

@owent Great! :) I got an answer from @lalitb in the slack channel thread earlier today:

This is interesting, I didn't realize initially we would have to deviate from header-only API to support DLL. Header only api is one of the basic requirements right from start of this project ( https://github.com/open-telemetry/opentelemetry-cpp/blob/v1.1.0/docs/requirements.md ). Supporting DLL was discussed in this week community meeting, and it was agreed to wait for more interest/requests for this, before deciding. But deviation from header-only API needs further thoughts, now that we are already v1.0.

To which I replied

Thanks for the quick response and raising this issue in the community meeting. :slightly_smiling_face: It makes me a bit sad that Windows/DLL users are not a priority, since I have the impression of opentelemetry aspiring to become a kind of basic observability infrastructure across languages and platforms (which I would love to be able to use). I realise, of course, that you are short on resources and must prioritise, but I am more than willing to contribute towards this goal. I just need a decision from you all on direction and how to tackle this before proceeding. I would really, really like to avoid having to maintain a fork of otel-cpp for this basic use case. Did you look at the article from Steve Ire (linked in the issue) where he makes both header-only and shared/static library possible? If (possibility of) header-only is a requirement, this might be a possible route to take. There are other ways to make both header-only and static/shared library possible as well. Another example of a library that provides both header-only and static/shared is https://github.com/fmtlib/fmt/blob/master/CMakeLists.txt (though I guess otel-cpp would have header-only be the default and static/shared be opt-in)

As I have written previously, I do not think a deviation from header-only is necessary, because we can provide the user with the choice of header-only (default), static or shared library. It could, perhaps, make the decision easier? :-)

And, if we do get a decision to proceed, I'll begin to work on a PR ASAP :-)

lalitb commented 2 years ago

Thanks, @meastp @owent for all the details above, and for bringing the clarity on changes required to support building DLLs. The priority is definitely equally for all the platforms and in the current setup

Again thanks @meastp for coming up to contribute DLL support for otel-api. And I do agree that supporting both header-only api and src files for DLLs would be one of the approaches we can follow. The only concern I see is the complexity it would add to the current codebase, and maintaining both of them simultaneously. We are already suffering from this when we added support for WITH_STL/WITH_ABSEIL options, the contributors for this have moved out of the project, and it's getting difficult to manage them with issues cropping for these libraries.

Again, nothing against Windows/DLL users as a priority. For me coming from Microsoft would definitely want this to be added :). The plan is to hear from the Windows users through this issue on the problems they have with the header-only / static-linking approach and decide accordingly. But then, this also needs to be discussed and decided in our community meeting ( and we encourage all to join there for getting wider feedback on this). Tagging @jsuereth @pyohannes @ThomsonTan if they want to add further.

meastp commented 2 years ago

@lalitb Thanks. I do sympathise with the complexity concern (in the WITH_STL case, I'm very happy to be able to be able to use this option, as WITH_ABSEIL and the absl library does not compile on MSVC projects with C++ standard > 17). I was hoping to at least try to use the techniques in the previous article I linked to have the additional complexity as low as possible.

I don't get why you need to hear from multiple Windows users that DLLs plain and simply do not work at all. A big warning "Windows dynamic link library (DLL) projects are not supported" in the README would be appropriate, I think. I also think (my personal experience) that developers of Windows DLLs are mostly developers of proprietary software, and not used to voicing their wishes/opinions in an open sourcey kind of way, leading to few or no responses. Instead of hearing from more Windows users, my concern is they'll just look elsewhere.

If I continue working on my test by incorporation the approach outlined for supporting both header-only and src files (and publish this as a branch (on my github fork) or draft PR, for example), would that give you more data to make a decision? :)

lalitb commented 2 years ago

If I continue working on my test by incorporation the approach outlined for supporting both header-only and src files (and publish this as a branch (on my github fork) or draft PR, for example), would that give you more data to make a decision? :)

That would definitely help bring more clarity. Thanks again.

meastp commented 2 years ago

@lalitb @owent Look at https://github.com/meastp/opentelemetry-cpp/tree/otel_support_shared_dll to see the changes required:

A new option to select API as header-only or not

option(WITH_HEADER_ONLY_API "Build the opentelemetry API as header-only instead of a compiled library" ON)

In api/CMakeLists.txt a conditional on this option (and a check to prevent non-working dll builds with MSVC/Windows):

if(WITH_HEADER_ONLY_API)
    add_library(opentelemetry_api INTERFACE)
    target_compile_definitions(opentelemetry_api INTERFACE OTEL_WITH_HEADER_ONLY_API)

    if(MSVC)
      if(BUILD_SHARED_LIBS)
        message(
          FATAL_ERROR
            "Building header-only API combined with dynamic link libraries is not supported. Turn off option WITH_HEADER_ONLY_API to build the API as a dynamic link library as well, or turn off BUILD_SHARED_LIBS."
        )
        endif()
    endif()
else()
    add_library(opentelemetry_api 
        include/opentelemetry/baggage/baggage.cc 
        include/opentelemetry/baggage/baggage_context.cc 
        include/opentelemetry/common/kv_properties.cc 
        include/opentelemetry/context/propagation/global_propagator.cc
        include/opentelemetry/context/runtime_context.cc 
        include/opentelemetry/metrics/provider.cc 
        include/opentelemetry/trace/noop.cc 
        include/opentelemetry/trace/provider.cc 
        include/opentelemetry/trace/trace_state.cc)

    target_compile_definitions(opentelemetry_api PUBLIC OTEL_EXPORT)
endif()

I have added new export.h-headers with the export definitions. Did try GenerateExportHeader, but I think it might be easier to define one of our own, which will not require workarounds for Bazel.

I added export macro where appropriate:

class OTEL_API NoopTracer final : public Tracer, public std::enable_shared_from_this<NoopTracer>
{
 // ...
};

I have placed the source files alongside the header files, and when compiling with OTEL_WITH_HEADER_ONLY_API (defined when WITH_HEADER_ONLY_API is enabled) the source files are included in the header files:

#ifdef OTEL_WITH_HEADER_ONLY_API
#  include "opentelemetry/trace/noop.cc"
#endif  // OTEL_WITH_HEADER_ONLY_API
// end of file

I used the configuration stdlib-x64-Debug, with most features turned off. I have added code to ensure that traces/spans works correctly across exe and libraries (this test is most important for windows dlls).

WITH_HEADER_ONLY_API BUILD_SHARED_LIBS examples/simple_example.exe Comment
ON TRUE N/A As expected, configure fails with: Building header-only API combined with dynamic link libraries is not supported. Turn off option WITH_HEADER_ONLY_API to build the API as a dynamic link library as well, or turn off BUILD_SHARED_LIBS.
ON FALSE OK
OFF FALSE OK
OFF TRUE OK

All in all, I don't think this approach requires too much overhead.

(I have a few vpkg-related changes to make it easier for me to build using vcpkg. I don't need this to be part of the changes merged in a PR)

lalitb commented 2 years ago

Thanks, @meastp. Also tagging @ThomsonTan @maxgolov for review.

meastp commented 2 years ago

Great, thanks. Tagging @steve-madden who wrote this in the slack-thread for this issue:

Also interested in this and willing to get involved. We have a 20 yr old Windows service with about 100 dynamically loaded modules. Did not look forward to linking in a static opentelemetry-cpp library in dozens of them including OTLP gRPC support or re-architecting to combine them into one large DLL.

esigo commented 2 years ago

@meastp thanks for your work. I was wondering how one_definition_rule is preserved in your example:

#ifdef OTEL_WITH_HEADER_ONLY_API
#  include "opentelemetry/trace/noop.cc"
#endif  // OTEL_WITH_HEADER_ONLY_API
// end of file
meastp commented 2 years ago

@esigo in the .cc file, I also use another macro OTEL_HEADER_ONLY_API_INLINE to add the inline specifier iff OTEL_WITH_HEADER_ONLY_API is defined and the .cc file is included from the header (look in opentelemetry/export.h).

This is the entire opentelemetry/trace/noop.cc file:

// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

#include "opentelemetry/trace/noop.h"
#include "opentelemetry/export.h"
#include "opentelemetry/version.h"

OPENTELEMETRY_BEGIN_NAMESPACE

namespace trace
{
OTEL_HEADER_ONLY_API_INLINE nostd::shared_ptr<Span> NoopTracer::StartSpan(
    nostd::string_view /*name*/,
    const common::KeyValueIterable & /*attributes*/,
    const SpanContextKeyValueIterable & /*links*/,
    const StartSpanOptions & /*options*/) noexcept
{
  // Don't allocate a no-op span for every StartSpan call, but use a static
  // singleton for this case.
  static nostd::shared_ptr<trace_api::Span> noop_span(
      new trace_api::NoopSpan{this->shared_from_this()});

  return noop_span;
}
}  // namespace trace
OPENTELEMETRY_END_NAMESPACE
github-actions[bot] commented 2 years ago

This issue was marked as stale due to lack of activity. It will be closed in 7 days if no furthur activity occurs.