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.

Sign up for GitHub

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

Fix GlobalLogHandler singleton creation order #1767

Merged
merged 3 commits into from
Nov 14, 2022
Merged

Fix GlobalLogHandler singleton creation order #1767

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
266f639
fix log handler creation order
lalitb Nov 13, 2022
a210c7a
include header
lalitb Nov 14, 2022
ad0415c
fix comment on the initialization order
lalitb Nov 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions sdk/include/opentelemetry/sdk/common/global_log_handler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,11 +134,10 @@ class GlobalLogHandler
OPENTELEMETRY_END_NAMESPACE

/**
* 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.
*/
* GlobalLogHandler and TracerProvider/MeterProvider/LoggerProvider are lazy sigletons.
* To ensure that GlobalLogHandler is the first one to be initialized (and so last to be
* destroyed), it is first used inside the constructors of TraceProvider, MeterProvider
* and LoggerProvider for debug logging. */
#define OTEL_INTERNAL_LOG_DISPATCH(level, message, attributes) \
do \
{ \
Expand Down
2 changes: 2 additions & 0 deletions sdk/src/logs/logger_provider.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#ifdef ENABLE_LOGS_PREVIEW

# include "opentelemetry/sdk/logs/logger_provider.h"
# include "opentelemetry/sdk/common/global_log_handler.h"

# include <memory>
# include <mutex>
Expand All @@ -26,6 +27,7 @@ LoggerProvider::LoggerProvider(std::unique_ptr<LogRecordProcessor> &&processor,
std::vector<std::unique_ptr<LogRecordProcessor>> processors;
processors.emplace_back(std::move(processor));
context_ = std::make_shared<sdk::logs::LoggerContext>(std::move(processors), std::move(resource));
OTEL_INTERNAL_LOG_DEBUG("[LoggerProvider] LoggerProvider created.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@lalitb lalitb Nov 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

LoggerProvider::LoggerProvider(std::vector<std::unique_ptr<LogRecordProcessor>> &&processors,
Expand Down
4 changes: 3 additions & 1 deletion sdk/src/metrics/meter_provider.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ MeterProvider::MeterProvider(std::shared_ptr<MeterContext> context) noexcept : c
MeterProvider::MeterProvider(std::unique_ptr<ViewRegistry> views,
sdk::resource::Resource resource) noexcept
: context_(std::make_shared<MeterContext>(std::move(views), resource))
{}
{
OTEL_INTERNAL_LOG_DEBUG("[MeterProvider] MeterProvider created.");
}

nostd::shared_ptr<metrics_api::Meter> MeterProvider::GetMeter(
nostd::string_view name,
Expand Down
5 changes: 4 additions & 1 deletion sdk/src/trace/tracer_provider.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// SPDX-License-Identifier: Apache-2.0

#include "opentelemetry/sdk/trace/tracer_provider.h"
#include "opentelemetry/sdk/common/global_log_handler.h"
#include "opentelemetry/sdk_config.h"

OPENTELEMETRY_BEGIN_NAMESPACE
Expand All @@ -14,7 +15,9 @@ namespace trace_api = opentelemetry::trace;

TracerProvider::TracerProvider(std::shared_ptr<sdk::trace::TracerContext> context) noexcept
: context_{context}
{}
{
OTEL_INTERNAL_LOG_DEBUG("[TracerProvider] TracerProvider created.");
}

TracerProvider::TracerProvider(std::unique_ptr<SpanProcessor> processor,
resource::Resource resource,
Expand Down