Fix GlobalLogHandler singleton creation order

[lalitb]

Fixes #1718

Changes

GlobalLogHandler and MeterProvider are lazy singletons. As of now, MeterProvider singleton is first initialized, followed by GlobalLogHandler (when the first message is logged). This order of initialization means GlobalLogHandler is destroyed before MeterProvider, which further results in segfault when trying to use the logger from MeterProvider destructor.

The easy solution would be to use GlobalLogHandler to log message from inside the MeterProvider constructor. This makes the complier to guarantee that the GlobalLogHandler is constructed before MeterProvider, and so destroyed after MeterProvider. Have tested the changes locally by reproducing the segfault, and then ensuring there is no segfault with the change.

To demonstrate the order of initialization, refer to the sample code here - https://godbolt.org/z/K8x1dcPW5

The order of initiation and destruction in sample is:

Log::Log
MeterProvider
MeterProvider::ForceFlush
~MeterProvider
~Log::Log

And order of initialization and destruction after replacing line 32 with std::cout << "MeterProvider\n" in the sample code is:

MeterProvider
Log::Log
MeterProvider::ForceFlush
~Log::Log
~MeterProvider

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

[codecov]

Codecov Report

Merging #1767 (ad0415c) into main (389b84f) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1767      +/-   ##
==========================================
- Coverage   85.79%   85.74%   -0.05%     
==========================================
  Files         171      171              
  Lines        5212     5214       +2     
==========================================
- Hits         4471     4470       -1     
- Misses        741      744       +3     

Impacted Files	Coverage Δ
...lude/opentelemetry/sdk/common/global_log_handler.h	72.23% <ø> (ø)
sdk/src/metrics/meter_provider.cc	91.18% <100.00%> (+0.27%)	⬆️
sdk/src/trace/tracer_provider.cc	77.28% <100.00%> (+0.53%)	⬆️
ext/src/http/client/curl/http_client_curl.cc	80.31% <0.00%> (-1.13%)	⬇️

[marcalff]

Looks ok to me.

Please also fix this comment:

/**
 * We can not decide the destroying order of signaltons.
 * Which means, the destructors of other singletons (GlobalLogHandler,TracerProvider and etc.)
 * may be called after destroying of global LogHandler and use OTEL_INTERNAL_LOG_* in it.We can do
 * nothing but ignore the log in this situation.
 */
#define OTEL_INTERNAL_LOG_DISPATCH(level, message, attributes)                            \

[lalitb]

Looks ok to me.

Please also fix this comment:

/**
 * We can not decide the destroying order of signaltons.
 * Which means, the destructors of other singletons (GlobalLogHandler,TracerProvider and etc.)
 * may be called after destroying of global LogHandler and use OTEL_INTERNAL_LOG_* in it.We can do
 * nothing but ignore the log in this situation.
 */
#define OTEL_INTERNAL_LOG_DISPATCH(level, message, attributes)                            \

Good point, have rephrased the comment now.

[ThomsonTan]

How about other ctors? Didn't add the log there because they are not used by lazy singleton pattern? Probably add the log for consistency as the log message is provider instance created.

[lalitb]

You mean to enhance our debug logging in general by adding logs in all the constructors? That's good feedback, will create a separate issue for it, as it is not directly related to this fix.

[marcalff]

How about other ctors? Didn't add the log there because they are not used by lazy singleton pattern? Probably add the log for consistency as the log message is provider instance created.

The per signal singletons are the entry points to the SDK. No other code in the SDK can be executed before building these per signal singletons.

So, by definition, once these singletons are executed, it is guaranteed that every other class used in the SDK implementation will see an already initialized global log handler.

There is not need, to fix this bug, to add log statements in every classes, even if another class uses a lazy singleton pattern.

[marcalff]

LGTM, thanks for the investigation and fix.

[marcalff]

How about other ctors? Didn't add the log there because they are not used by lazy singleton pattern? Probably add the log for consistency as the log message is provider instance created.

The per signal singletons are the entry points to the SDK. No other code in the SDK can be executed before building these per signal singletons.

So, by definition, once these singletons are executed, it is guaranteed that every other class used in the SDK implementation will see an already initialized global log handler.

There is not need, to fix this bug, to add log statements in every classes, even if another class uses a lazy singleton pattern.

[johanpel]

Thanks @lalitb for the fix!