Skip to content
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

Header only singletons are not working properly for Windows #2534

Open
marcalff opened this issue Feb 14, 2024 · 5 comments
Open

Header only singletons are not working properly for Windows #2534

marcalff opened this issue Feb 14, 2024 · 5 comments
Labels
bug Something isn't working triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@marcalff
Copy link
Member

marcalff commented Feb 14, 2024

The opentelemetry-cpp API uses header only singletons, for example for the tracer provider.

When:

  • an application calls TracerProvider::SetTracerProvider() in the main application code
  • a shared library is loaded by the main application
  • the shared library uses TracerProvider::GetTracerProvider()

the shared library does not see the tracer provider installed in the main application.

As a result, opentelemetry is disabled in the shared library (the noop tracer provider is seen instead).

Related issues:

The issue about singletons for the Windows platform is known, but still not resolved.

@marcalff marcalff added the bug Something isn't working label Feb 14, 2024
@github-actions github-actions bot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Feb 14, 2024
@ThomsonTan ThomsonTan added triage/accepted Indicates an issue or PR is ready to be actively worked on. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Feb 21, 2024
@marcalff marcalff removed the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Feb 22, 2024
@malkia
Copy link

malkia commented Feb 22, 2024

I've been running the singleton_test in my soft-fork that targets single .dll for Windows (combining sdk+api) - and it's passing in that version. - https://github.com/malkia/opentelemetry-cpp/actions/runs/7878684091/job/21497437677#step:5:8420 - this has been my "test" to ensure that I'm not breaking things across dlls - but I've been relyin on bazel to make the .dll, not CMake (it's what I'm more familiar with).

Copy link

This issue was marked as stale due to lack of activity.

@marcalff
Copy link
Member Author

marcalff commented Jan 29, 2025

As of 2025, this issue is still unresolved.

Possible options to resolve this discussed below.

Option 1, make header-only singletons work

This has been attempted before, but failed.

The goal is to find a proper compile option and/or linker annotations to force duplicate copies in each library to be resolved by the linker.

This assumes the executable binary format supports this (Linux does).

Option 2, give up on header-only

Singletons are implemented in an opentelemetry-api DLL.

The major drawback is that every library instrumenting for opentelemetry-cpp now must link with this new library, which causes some impact on builds, possibly preventing adoption of opentelemetry-cpp.

This is technically easy to implement, but not desirable.

Option 3, give-up on singleton

In this scenario, each library has a local copy of various providers, like trace::Provider.

When calling Provider::SetTracerProvider, opentelemetry is to:

  • Somehow find every library loaded in the current process
  • Inspect each library to see if a copy of trace::Provider exists
  • Set the local copy to the proper value.

This also means to somehow be notified when a library is loaded, to adjust the providers copy.

This sounds very risky technically, and very dependent on Windows.

@github-actions github-actions bot removed the Stale label Jan 30, 2025
@malkia
Copy link

malkia commented Jan 31, 2025

There is one more option, call it 2b, not really a great option, but the main .exe-cutable can export symbols (while not a typical thing, it's there and it's used across - like Microsoft's Agility DirectX SDK, NVIDIA, AMD to hint the driver about the process).

Here is the example from the Agility SDK - https://devblogs.microsoft.com/directx/gettingstarted-dx12agility/#parametersa

This would require the main application, to compile a small subset of the .h-eader only files, maybe define some #define OTEL_COMPILE_SINGLETONS (betterly named), and expose these symbols, such that other open-telemetry dlls can see them.

Now the limitation comes from not being able to use this in a plugin - e.g. a Maya / Max / Adobe, or any other system, or language relying on .dll to do ffi - Java / Python / etc, because one can't touch python.exe - and then overall the complexity of this.

@malkia
Copy link

malkia commented Jan 31, 2025

Option 3 sounds like most promising, we can probably find for each major OS some primitive to have a process-level globally named object that holds the singleton. Some helper class, or library (individually tested), that can provide/create such singletons, like a little "key/value" db.

My biggest issue here, is that by being a header-only library, more or less we'll have to expose some OS-specific innards (like windows.h) where such headers would have to be included (or alternatively functions needed re-defined (ugh)). Now that in a way can possibly also follow idea from above, where such "os"-specific innards, while still in a .h file can be hidden, by asking users to compile at least one specific .h as part of their .cpp (like include it at the bottom, and #define OTEL_COMPILE_SINGLETONS), not ideal but not the end of the world.

This would still work for plugins, but another issue raises there - if you say have two plugins, or in Java / Python / etc., two compiled C++ libraries used from ffi or some other ways, but these happen to use a bit different version of OpenTelemetry (still same "abi") - they have to communicate whether the singleton internals are still same abi - in a way this means that the singletons would become part of the abi (I guess that was already implied one way or another).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

3 participants