Fix singletons across DLL boundaries

[Desp4]

Fixes #2534

Changes

This is my proposed solution to the dreaded issue #2534 with singletons in DLLs. I will briefly list the changes below, then attempt to defend my case for this specific approach.

• The changes only concern Windows, Linux targets are unaffected
• The singleton retrieval in API(opentelemetry::api) on Windows has changed, but it has remained header-only
• opentelemetry::api_static target added to not leak the Windows.h header. This is a convenience target, it does not replace opentelemetry::api, but is optionally there for those in need.
• opentelemetry::abi_symbol_bridge shared target(DLL) added to facilitate symbol injection. Neither the API nor SDK depend on it, instead an end-user target willing to support singletons in DLLs must link it(no additional C++ code necessary).

As the consequence of these changes, all singleton tests now pass on Windows(RUN_FAILING_WINDOWS_TEST has been removed).

The current state of the branch

I have only changed the source code and relevant CMake instructions. That is, I have not updated the Bazel build, documentation and CI(if those need to be updated).

I am willing to update all of those as well if this is the solution the community would accept.

My proposed solution, an explanation

I am submitting this code as I see it as the solution that covers the use-cases of the API/SDK as well as keeps the overhead on API users low, mainly:

• The API remains header-only.
• Singleton retrieval remains thread-safe, as the symbol bridge injection is atomic.
• The singleton retrieval remains low-cost both as a no-op and with SDK-injected entities(discussed below).
• Enabling singleton support in DLLs is opt-in and simple, requiring only an additional target being linked in.
• Not inconvenient on the SDK side. Supposedly if the consumer of the SDK wants to support API usage in DLLs, linking and shipping an additional DLL to enable this functionality is not a deal-breaker.

The points discussed above are critical for my use-cases, and this is the solution I am personally going with for myself. Below I will briefly explain what changes to singleton symbol lookup have been made.

Singleton retrieval mechanism on Windows

This is a description of the logic happening in symbol_bridge_windows.h:LoadSymbolBridgeSymbol:

• An ATOM is checked if it had been raised. If not, the current singleton retrieval mechanism is used. This is the no-op case, the added cost here is ATOM lookup, which from my measurements is low enough to not be critical(measured around 30ns on my machine).
• If an ATOM is raised, perform a load of opentelemetry::abi_symbol_bridge via GetModuleHandleEx. Crucially this is not a LoadLibrary call as to not introduce the whole headache of DLL lookup on the API side. That is, the library has to already have been loaded into the process.
• If the bridge library is loaded(under normal conditions, this is always the case), all further singleton calls go to the bridge DLL instead, now sharing the same object.

The responsibility on loading opentelemetry::abi_symbol_bridge into the process is on the consumer of the SDK. I have chosen the route of linking it to the desired binary, opentelemetry::abi_symbol_bridge configured in CMake in such a way as to implicitly cause a load without any calls to it from user code directly.

The mechanism above covers all the cases of singleton injection(or at least the ones that are important to me):

• SDK is initialized in the main executable
• SDK is initialized in a linked DLL
• SDK is initialized in a dynamically loaded DLL, before or after API calls have already been made

What I'm asking for

I would like the community to review my proposed solution and whether it is something that aligns with their view for the project. If these changes are acceptable, I am willing to take additional feedback on the work already done, as well as on the work that still needs to be done for an approved merge.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: Desp4 / name: Desp4 (a004844, d270e52)

[marcalff]

This is my proposed solution to the dreaded issue #2534 with singletons in DLLs.

Dreaded indeed, thanks for looking onto this one.

This definitively gets our attention ;-)

Since you are a first time contributor, please find a typo in the code or comments, and file a PR for it.
The point is to make you a contributor, so CI will start instead of waiting for maintainers to manually start the workflow for each commit.

This will help you work out CI issues for this PR, if any.

[Desp4]

Thanks, I may dedicate some time later to maybe scan through the repo in search of some quick typo fix. But this is simply for the pipelines to recognize me as a contributor and start on their own, right? That is, this is not a requirement for my pull request to be looked into

[marcalff]

Thanks, I may dedicate some time later to maybe scan through the repo in search of some quick typo fix. But this is simply for the pipelines to recognize me as the maintainer and start on their own, right?

For the pipeline to recognize you as existing contributor and start right away, yes.

That is, this is not a requirement for my pull request to be looked into

Correct, review will start anyway.

This is to help you see the result of CI sooner, to make adjustments in necessary ... for example clang-format fails right now.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.78%. Comparing base (cdb5e44) to head (5678bed).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4642      +/-   ##
==========================================
+ Coverage   86.77%   86.78%   +0.01%     
==========================================
  Files         525      525              
  Lines       20747    20747              
==========================================
+ Hits        18002    18004       +2     
+ Misses       2745     2743       -2     

Files with missing lines	Coverage Δ
api/include/opentelemetry/baggage/baggage.h	98.28% <ø> (ø)
...ntelemetry/context/propagation/global_propagator.h	100.00% <ø> (ø)
...pi/include/opentelemetry/context/runtime_context.h	92.14% <ø> (ø)
api/include/opentelemetry/logs/provider.h	100.00% <ø> (ø)
api/include/opentelemetry/metrics/provider.h	100.00% <ø> (ø)
api/include/opentelemetry/trace/provider.h	100.00% <ø> (ø)
api/include/opentelemetry/trace/trace_state.h	98.95% <ø> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[marcalff]

@Desp4

Thanks for the contribution, this looks very promising.

Please investigate the current CI failures on Windows, the unit test singleton.SingletonTest.Uniqueness is failing, and this test is precisely testing the issue at hand.

@lalitb @ThomsonTan @owent

Please review, this is windows specific.

[Desp4]

Added the missing env path for the windows test pipeline.
Also remove the static flavor of the api library I mentioned in the pull request: linking both it and anything from the SDK would produce an ODR violation as it currently is(since the SDK links the inlined functions). I've left the preprocessor toggles in place for such a library to be written consumer-side if need be.

[Desp4]

The pipelines should be good now. The bazel build and the docs remain unchanged

[owent]

Shouldn't the otel API be header-only?
We discussed singleton implementations before in #1105 and #1520, but I don't think we've found a perfect solution yet.
Even if we allow source files for the otel API, if users build it as a static library and link it into more than one DLL or executable, it may still produce more than one singleton object.

[Desp4]

Shouldn't the otel API be header-only?

The API remains header-only, this has not changed

[owent]

Just wondersing, Should this return {nullptr, false} here?

Two concerns about the lazy-loading path:

• If the user doesn't load opentelemetry_api_symbol_bridge, FindAtomW runs on every singleton access. I don't know how costly that is, but it's a per-call overhead we may want to avoid.
• If we support LoadLibrary, the cached address in static LoadedBridgeSymbol symbol{nullptr, true}; becomes invalid after FreeLibrary, which forces us to track the DLL lifetime and reset all the singletons — that adds real complexity.

Given that, I'd lean toward requiring the bridge to be linked rather than dynamically loaded. Then its availability is fixed when this function is called (always present or always absent), and we can drop the reload logic for otelcpp-abi-bridge. WDYT?

[Desp4]

• I measured the cost of FindAtomW call to be around 30ns on my machine, plus this should lend itself well for branch prediction in a repeated noop case when the atom is not found. I personally don't think that the added cost is critical enough, and I also don't know of a cheaper process-wide method to check for a loaded abi bridge(though this is of course irrelevant if we drop the flexibility of the bridge being loaded after the first singleton access).
• How so? The FreeLibrary call is tied to the lifetime of ModuleLifetime bridge_module, that is we're only freeing the library at the global destruction phase when the last binary using the bridge is getting unloaded(since FreeLibrary only decrements the refcount).

My thoughts on dropping the dynamic check for the bridge availability. I personally don't find the dynamic check to be a critical feature and wouldn't oppose dropping it, though the benefit I see is only cutting the cost of repeated atom check for a noop case.

I originally wanted to support the dynamic bridge load specifically for two cases:

1. If some API consumer makes singleton calls at library load time(global inits). In this case the API consumer uses the singleton before the main executable has initialized them, thus going to the "bridge not present" path and caching it, forever being in the noop case.
   I find this case niche enough to not be something the SDK ought to support(given that using the API at global init is very much guaranteed to only give you the noop singletons even on linux, this usage pattern is not really productive to begin with).
2. The case of the SDK being initialized in a deferred runtime-loaded library, after some API consuming calls have already been made.
   This too I don't find to be that critical since most plugin-loading software loads them near startup before any API consuming code is supposedly ran.

So overall I would not oppose dropping the atom check and just resorting to a hard requirement of a bridge being there on first singleton access, given that the cases I could come up with above are not that common or worth supporting.