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 global log handle symbols when using dlopen #1420

Merged
merged 4 commits into from
Jun 4, 2022
+87 −32
Merged

Fix global log handle symbols when using dlopen #1420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f1d3c9b
Fix global log handle symbols when using dlopen
owent Jun 1, 2022
2d18cf0
Fix format for bazel files
owent Jun 1, 2022
f756ff3
Add dependency for jaeger exporter
owent Jun 1, 2022
7d88b91
Keep `NoopLogHandler::Handle` empty
owent Jun 2, 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
4 changes: 4 additions & 0 deletions .gitattributes
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,7 @@ LICENSE* text
## git files
.gitignore text eol=lf
.gitattributes text eol=lf

## bazel files
WORKSPACE text eol=lf
BUILD text eol=lf
1 change: 1 addition & 0 deletions exporters/jaeger/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ cc_library(
tags = ["jaeger"],
deps = [
":jaeger_exporter",
"//sdk/src/common:global_log_handler",
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to add this for other exporters too. Eg, zipkin exporter includes sdk_config.h, which further includes global_log_handler.

Copy link
Member Author

Choose a reason for hiding this comment

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

Exporters shoud denpend on //sdk/src/logs , //sdk/src/metrics or //sdk/src/trace , which already depend //sdk/src/common:global_log_handler

],
)

Expand Down
39 changes: 8 additions & 31 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 @@ -55,7 +55,7 @@ inline std::string LevelToString(LogLevel level)
class LogHandler
{
public:
virtual ~LogHandler() = default;
virtual ~LogHandler();

virtual void Handle(LogLevel level,
const char *file,
Expand All @@ -71,22 +71,7 @@ class DefaultLogHandler : public LogHandler
const char *file,
int line,
const char *msg,
const sdk::common::AttributeMap &attributes) noexcept override
{
std::stringstream output_s;
output_s << "[" << LevelToString(level) << "] ";
if (file != nullptr)
{
output_s << "File: " << file << ":" << line;
}
if (msg != nullptr)
{
output_s << msg;
}
output_s << std::endl;
// TBD - print attributes
std::cout << output_s.str(); // thread safe.
}
const sdk::common::AttributeMap &attributes) noexcept override;
};

class NoopLogHandler : public LogHandler
Expand All @@ -96,10 +81,7 @@ class NoopLogHandler : public LogHandler
const char *file,
int line,
const char *msg,
const sdk::common::AttributeMap &error_attributes) noexcept override
{
// ignore the log message
}
const sdk::common::AttributeMap &error_attributes) noexcept override;
};

/**
Expand All @@ -113,7 +95,7 @@ class GlobalLogHandler
*
* By default, a default LogHandler is returned.
*/
static const nostd::shared_ptr<LogHandler> &GetLogHandler() noexcept
static inline const nostd::shared_ptr<LogHandler> &GetLogHandler() noexcept
{
return GetHandlerAndLevel().first;
}
Expand All @@ -123,7 +105,7 @@ class GlobalLogHandler
* This should be called once at the start of application before creating any Provider
* instance.
*/
static void SetLogHandler(nostd::shared_ptr<LogHandler> eh) noexcept
static inline void SetLogHandler(nostd::shared_ptr<LogHandler> eh) noexcept
{
GetHandlerAndLevel().first = eh;
}
Expand All @@ -133,22 +115,17 @@ class GlobalLogHandler
*
* By default, a default log level is returned.
*/
static LogLevel GetLogLevel() noexcept { return GetHandlerAndLevel().second; }
static inline LogLevel GetLogLevel() noexcept { return GetHandlerAndLevel().second; }

/**
* Changes the singleton Log level.
* This should be called once at the start of application before creating any Provider
* instance.
*/
static void SetLogLevel(LogLevel level) noexcept { GetHandlerAndLevel().second = level; }
static inline void SetLogLevel(LogLevel level) noexcept { GetHandlerAndLevel().second = level; }

private:
static std::pair<nostd::shared_ptr<LogHandler>, LogLevel> &GetHandlerAndLevel() noexcept
{
static std::pair<nostd::shared_ptr<LogHandler>, LogLevel> handler_and_level{
nostd::shared_ptr<LogHandler>(new DefaultLogHandler), LogLevel::Warning};
return handler_and_level;
}
static std::pair<nostd::shared_ptr<LogHandler>, LogLevel> &GetHandlerAndLevel() noexcept;
};

} // namespace internal_log
Expand Down
12 changes: 12 additions & 0 deletions sdk/src/common/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,18 @@ cc_library(
include_prefix = "src/common",
deps = [
"//api",
"//sdk:headers",
"//sdk/src/common/platform:fork",
],
)

cc_library(
name = "global_log_handler",
srcs = [
"global_log_handler.cc",
],
deps = [
"//api",
"//sdk:headers",
],
)
Comment on lines +35 to +44
Copy link
Member

Choose a reason for hiding this comment

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

I am not familiar with BAZEL, and I am confused by the build script here.

The bazel build defines two libraries, "random" and "global_log_handler", while the CMake build defines only one library, named "opentelemetry_common"

This is a pre existing issue, but how come file random.cc is compiled in library "random" with bazel, and "opentelemetry_common" with CMake ?

I would expect both build to produce the same libraries with the same name, and put the same *.cc code in the same *.so library.

Copy link
Member

Choose a reason for hiding this comment

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

Agree, though not related to this PR, it would be good to have both cmake and bazel produce a consistent set of libraries.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree too. We can raise another PR to produce a consistent set of libraries between bazel and cmake.There are more libraries names need be reviewed.

2 changes: 1 addition & 1 deletion sdk/src/common/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
set(COMMON_SRCS random.cc core.cc)
set(COMMON_SRCS random.cc core.cc global_log_handler.cc)
if(WIN32)
list(APPEND COMMON_SRCS platform/fork_windows.cc)
else()
Expand Down
57 changes: 57 additions & 0 deletions sdk/src/common/global_log_handler.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// Copyright The OpenTelemetry Authors
// SPDX-License-Identifier: Apache-2.0

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

#include <cstring>
#include <random>

OPENTELEMETRY_BEGIN_NAMESPACE
namespace sdk
{
namespace common
{
namespace internal_log
{

LogHandler::~LogHandler() {}

void DefaultLogHandler::Handle(LogLevel level,
const char *file,
int line,
const char *msg,
const sdk::common::AttributeMap &attributes) noexcept
{
std::stringstream output_s;
output_s << "[" << LevelToString(level) << "] ";
if (file != nullptr)
{
output_s << "File: " << file << ":" << line;
}
if (msg != nullptr)
{
output_s << msg;
}
output_s << std::endl;
// TBD - print attributes
std::cout << output_s.str(); // thread safe.
}

void NoopLogHandler::Handle(LogLevel,
const char *,
int,
const char *,
const sdk::common::AttributeMap &) noexcept
{}

std::pair<nostd::shared_ptr<LogHandler>, LogLevel> &GlobalLogHandler::GetHandlerAndLevel() noexcept
{
static std::pair<nostd::shared_ptr<LogHandler>, LogLevel> handler_and_level{
nostd::shared_ptr<LogHandler>(new DefaultLogHandler), LogLevel::Warning};
Comment on lines +49 to +50
Copy link
Member

Choose a reason for hiding this comment

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

The singleton is in the *.cc file now, great.

As a side note, I reviewed every occurrence of the static keyword in every header file under sdk/include.

There are a few other static variables used, but none of them are required to be unique, so the scope of the fix is really narrowed down to this one alone, handler_and_level in GlobalLogHandler::GetHandlerAndLevel().

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we can raise another issue to move all static variables into *.cc, it's still a waste of memory.

Copy link
Member

Choose a reason for hiding this comment

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

Agree, filed issue #1429 then.

return handler_and_level;
}

} // namespace internal_log
} // namespace common
} // namespace sdk
OPENTELEMETRY_END_NAMESPACE
1 change: 1 addition & 0 deletions sdk/src/logs/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ cc_library(
deps = [
"//api",
"//sdk:headers",
"//sdk/src/common:global_log_handler",
"//sdk/src/resource",
],
)
1 change: 1 addition & 0 deletions sdk/src/metrics/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ cc_library(
deps = [
"//api",
"//sdk:headers",
"//sdk/src/common:global_log_handler",
"//sdk/src/resource",
],
)
1 change: 1 addition & 0 deletions sdk/src/trace/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ cc_library(
deps = [
"//api",
"//sdk:headers",
"//sdk/src/common:global_log_handler",
"//sdk/src/common:random",
"//sdk/src/resource",
],
Expand Down
1 change: 1 addition & 0 deletions sdk/test/common/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,7 @@ cc_test(
deps = [
"//api",
"//sdk:headers",
"//sdk/src/common:global_log_handler",
"@com_google_googletest//:gtest_main",
],
)
Expand Down