From a064d191597eb90efb440334d34f8d5786cef114 Mon Sep 17 00:00:00 2001 From: Seufert Date: Mon, 23 Nov 2020 17:16:11 -0700 Subject: [PATCH 01/21] Moved log record from stack to heap, removed const --- api/include/opentelemetry/logs/logger.h | 18 ++++++------ api/include/opentelemetry/logs/noop.h | 2 +- api/test/logs/logger_test.cc | 31 ++++++++++++--------- sdk/include/opentelemetry/sdk/logs/logger.h | 3 +- sdk/src/logs/logger.cc | 13 ++------- sdk/test/logs/logger_sdk_test.cc | 8 +++--- 6 files changed, 37 insertions(+), 38 deletions(-) diff --git a/api/include/opentelemetry/logs/logger.h b/api/include/opentelemetry/logs/logger.h index 5f22066460..11720a3831 100644 --- a/api/include/opentelemetry/logs/logger.h +++ b/api/include/opentelemetry/logs/logger.h @@ -59,7 +59,7 @@ class Logger * A default LogRecord that will be assigned if no parameters are passed to Logger's .log() method * which should at minimum assign the trace_id, span_id, and timestamp */ - virtual void log(const LogRecord &record) noexcept = 0; + virtual void log(nostd::shared_ptr record) noexcept = 0; /** Overloaded methods for unstructured logging **/ inline void log(nostd::string_view message) noexcept @@ -79,10 +79,10 @@ class Logger core::SystemTimestamp time) noexcept { // creates a LogRecord object with given parameters, then calls log(LogRecord) - LogRecord r; - r.severity = severity; - r.body = message; - r.timestamp = time; + auto r = nostd::shared_ptr(new LogRecord); + r->severity = severity; + r->body = message; + r->timestamp = time; log(r); } @@ -104,10 +104,10 @@ class Logger const common::KeyValueIterable &attributes) noexcept { // creates a LogRecord object with given parameters, then calls log(LogRecord) - LogRecord r; - r.severity = severity; - r.name = name; - r.attributes = attributes; + auto r = nostd::shared_ptr(new LogRecord); + r->severity = severity; + r->name = name; + r->attributes = attributes; log(r); } diff --git a/api/include/opentelemetry/logs/noop.h b/api/include/opentelemetry/logs/noop.h index 3184baa3c6..fa82714ceb 100644 --- a/api/include/opentelemetry/logs/noop.h +++ b/api/include/opentelemetry/logs/noop.h @@ -41,7 +41,7 @@ class NoopLogger final : public Logger public: NoopLogger() = default; - void log(const LogRecord &record) noexcept override {} + void log(nostd::shared_ptr record) noexcept override {} }; /** diff --git a/api/test/logs/logger_test.cc b/api/test/logs/logger_test.cc index 937ece860a..9c135aba42 100644 --- a/api/test/logs/logger_test.cc +++ b/api/test/logs/logger_test.cc @@ -23,35 +23,40 @@ TEST(Logger, GetLoggerDefault) } TEST(Logger, GetNoopLoggerName) -{ - auto lp = Provider::GetLoggerProvider(); - auto logger = lp->GetLogger("TestLogger"); -} - -TEST(Logger, GetNoopLoggerNameWithArgs) { auto lp = Provider::GetLoggerProvider(); + // Get a logger with no arguments + auto logger1 = lp->GetLogger("TestLogger1"); + + // Get a logger with options passed + auto logger2 = lp->GetLogger("TestLogger2", "Options"); + + // Get a logger with arguments std::array sv{"string"}; span args{sv}; - auto logger = lp->GetLogger("NoopLoggerWithArgs", args); - // should probably also test that arguments were set properly too - // by adding a getArgs() method in NoopLogger + auto logger3 = lp->GetLogger("TestLogger3", args); } -TEST(Logger, NoopLog) +TEST(Logger, LogMethod) { auto lp = Provider::GetLoggerProvider(); auto logger = lp->GetLogger("TestLogger"); - LogRecord r; - r.name = "Noop log name"; + + // Test log(severity, name) method + logger->log(Severity::kError, "Error message"); + + // Test log(LogRecord) + auto r = opentelemetry::nostd::shared_ptr(new LogRecord); + r->name = "Log Record"; + r->severity = Severity::kInfo; logger->log(r); } // Define a basic Logger class class TestLogger : public Logger { - void log(const LogRecord &record) noexcept override {} + void log(opentelemetry::nostd::shared_ptr record) noexcept override {} }; // Define a basic LoggerProvider class that returns an instance of the logger class defined above diff --git a/sdk/include/opentelemetry/sdk/logs/logger.h b/sdk/include/opentelemetry/sdk/logs/logger.h index 4b49351030..e38f95ebeb 100644 --- a/sdk/include/opentelemetry/sdk/logs/logger.h +++ b/sdk/include/opentelemetry/sdk/logs/logger.h @@ -47,7 +47,8 @@ class Logger final : public opentelemetry::logs::Logger * Writes a log record into the processor. * @param record The record to write into the processor. */ - void log(const opentelemetry::logs::LogRecord &record) noexcept override; + void log( + opentelemetry::nostd::shared_ptr record) noexcept override; private: // The logger provider of this Logger. Uses a weak_ptr to avoid cyclic dependancy issues the with diff --git a/sdk/src/logs/logger.cc b/sdk/src/logs/logger.cc index bc19f5d1c1..b129a742fc 100644 --- a/sdk/src/logs/logger.cc +++ b/sdk/src/logs/logger.cc @@ -25,7 +25,7 @@ Logger::Logger(std::shared_ptr logger_provider) noexcept : logger_provider_(logger_provider) {} -void Logger::log(const opentelemetry::logs::LogRecord &record) noexcept +void Logger::log(opentelemetry::nostd::shared_ptr record) noexcept { // If this logger does not have a processor, no need to create a log record auto processor = logger_provider_.lock()->GetProcessor(); @@ -35,15 +35,8 @@ void Logger::log(const opentelemetry::logs::LogRecord &record) noexcept } // TODO: Sampler logic (should include check for minSeverity) - - /** - * Convert the LogRecord to the heap first before sending to processor. - * TODO: Change the API log(LogRecord) function to log(*LogRecord) so the following line - * converting record a heap variable can be removed - */ - auto record_pointer = - std::unique_ptr(new opentelemetry::logs::LogRecord(record)); - + auto record_pointer = std::unique_ptr( + new opentelemetry::logs::LogRecord(*record.get())); // TODO: Do not want to overwrite user-set timestamp if there already is one - // add a flag in the API to check if timestamp is set by user already before setting timestamp diff --git a/sdk/test/logs/logger_sdk_test.cc b/sdk/test/logs/logger_sdk_test.cc index 6c36b68c0c..e17d7cc356 100644 --- a/sdk/test/logs/logger_sdk_test.cc +++ b/sdk/test/logs/logger_sdk_test.cc @@ -30,8 +30,8 @@ TEST(LoggerSDK, LogToNullProcessor) auto logger = lp->GetLogger("logger"); // Log a sample log record to a nullptr processor - opentelemetry::logs::LogRecord r; - r.name = "Test log"; + auto r = std::shared_ptr(new opentelemetry::logs::LogRecord); + r->name = "Test log"; logger->log(r); } @@ -65,7 +65,7 @@ TEST(LoggerSDK, LogToAProcessor) // and that the logger's processor is the same as lp's processor // Log a sample log record to the processor - opentelemetry::logs::LogRecord r; - r.name = "Test log"; + auto r = std::shared_ptr(new opentelemetry::logs::LogRecord); + r->name = "Test log"; logger->log(r); } From cea306c500bbb9ace3ed12d89e9151b5e4a8943f Mon Sep 17 00:00:00 2001 From: Seufert Date: Mon, 23 Nov 2020 17:52:52 -0700 Subject: [PATCH 02/21] Changed log to Log (camelcase) --- api/include/opentelemetry/logs/logger.h | 22 ++++++++++----------- api/include/opentelemetry/logs/noop.h | 2 +- api/test/logs/logger_test.cc | 6 +++--- sdk/include/opentelemetry/sdk/logs/logger.h | 2 +- sdk/src/logs/logger.cc | 2 +- sdk/test/logs/logger_sdk_test.cc | 4 ++-- 6 files changed, 19 insertions(+), 19 deletions(-) diff --git a/api/include/opentelemetry/logs/logger.h b/api/include/opentelemetry/logs/logger.h index 11720a3831..d0e1583d74 100644 --- a/api/include/opentelemetry/logs/logger.h +++ b/api/include/opentelemetry/logs/logger.h @@ -59,22 +59,22 @@ class Logger * A default LogRecord that will be assigned if no parameters are passed to Logger's .log() method * which should at minimum assign the trace_id, span_id, and timestamp */ - virtual void log(nostd::shared_ptr record) noexcept = 0; + virtual void Log(nostd::shared_ptr record) noexcept = 0; /** Overloaded methods for unstructured logging **/ - inline void log(nostd::string_view message) noexcept + inline void Log(nostd::string_view message) noexcept { // Set severity to the default then call log(Severity, String message) method - log(Severity::kDefault, message); + Log(Severity::kDefault, message); } - inline void log(Severity severity, nostd::string_view message) noexcept + inline void Log(Severity severity, nostd::string_view message) noexcept { // TODO: set default timestamp later (not in API) - log(severity, message, core::SystemTimestamp(std::chrono::system_clock::now())); + Log(severity, message, core::SystemTimestamp(std::chrono::system_clock::now())); } - inline void log(Severity severity, + inline void Log(Severity severity, nostd::string_view message, core::SystemTimestamp time) noexcept { @@ -84,7 +84,7 @@ class Logger r->body = message; r->timestamp = time; - log(r); + Log(r); } /** Overloaded methods for structured logging**/ @@ -92,14 +92,14 @@ class Logger // empty logs template ::value> * = nullptr> - inline void log(Severity severity = Severity::kDefault, + inline void Log(Severity severity = Severity::kDefault, nostd::string_view name = "", const T &attributes = {}) noexcept { - log(severity, name, common::KeyValueIterableView(attributes)); + Log(severity, name, common::KeyValueIterableView(attributes)); } - inline void log(Severity severity, + inline void Log(Severity severity, nostd::string_view name, const common::KeyValueIterable &attributes) noexcept { @@ -109,7 +109,7 @@ class Logger r->name = name; r->attributes = attributes; - log(r); + Log(r); } // TODO: add function aliases such as void debug(), void trace(), void info(), etc. for each diff --git a/api/include/opentelemetry/logs/noop.h b/api/include/opentelemetry/logs/noop.h index fa82714ceb..cf41f63015 100644 --- a/api/include/opentelemetry/logs/noop.h +++ b/api/include/opentelemetry/logs/noop.h @@ -41,7 +41,7 @@ class NoopLogger final : public Logger public: NoopLogger() = default; - void log(nostd::shared_ptr record) noexcept override {} + void Log(nostd::shared_ptr record) noexcept override {} }; /** diff --git a/api/test/logs/logger_test.cc b/api/test/logs/logger_test.cc index 9c135aba42..2667aa6b2a 100644 --- a/api/test/logs/logger_test.cc +++ b/api/test/logs/logger_test.cc @@ -44,19 +44,19 @@ TEST(Logger, LogMethod) auto logger = lp->GetLogger("TestLogger"); // Test log(severity, name) method - logger->log(Severity::kError, "Error message"); + logger->Log(Severity::kError, "Error message"); // Test log(LogRecord) auto r = opentelemetry::nostd::shared_ptr(new LogRecord); r->name = "Log Record"; r->severity = Severity::kInfo; - logger->log(r); + logger->Log(r); } // Define a basic Logger class class TestLogger : public Logger { - void log(opentelemetry::nostd::shared_ptr record) noexcept override {} + void Log(opentelemetry::nostd::shared_ptr record) noexcept override {} }; // Define a basic LoggerProvider class that returns an instance of the logger class defined above diff --git a/sdk/include/opentelemetry/sdk/logs/logger.h b/sdk/include/opentelemetry/sdk/logs/logger.h index e38f95ebeb..48828bcf0e 100644 --- a/sdk/include/opentelemetry/sdk/logs/logger.h +++ b/sdk/include/opentelemetry/sdk/logs/logger.h @@ -47,7 +47,7 @@ class Logger final : public opentelemetry::logs::Logger * Writes a log record into the processor. * @param record The record to write into the processor. */ - void log( + void Log( opentelemetry::nostd::shared_ptr record) noexcept override; private: diff --git a/sdk/src/logs/logger.cc b/sdk/src/logs/logger.cc index b129a742fc..9556eb6b0e 100644 --- a/sdk/src/logs/logger.cc +++ b/sdk/src/logs/logger.cc @@ -25,7 +25,7 @@ Logger::Logger(std::shared_ptr logger_provider) noexcept : logger_provider_(logger_provider) {} -void Logger::log(opentelemetry::nostd::shared_ptr record) noexcept +void Logger::Log(opentelemetry::nostd::shared_ptr record) noexcept { // If this logger does not have a processor, no need to create a log record auto processor = logger_provider_.lock()->GetProcessor(); diff --git a/sdk/test/logs/logger_sdk_test.cc b/sdk/test/logs/logger_sdk_test.cc index e17d7cc356..1265bfc183 100644 --- a/sdk/test/logs/logger_sdk_test.cc +++ b/sdk/test/logs/logger_sdk_test.cc @@ -32,7 +32,7 @@ TEST(LoggerSDK, LogToNullProcessor) // Log a sample log record to a nullptr processor auto r = std::shared_ptr(new opentelemetry::logs::LogRecord); r->name = "Test log"; - logger->log(r); + logger->Log(r); } class DummyProcessor : public LogProcessor @@ -67,5 +67,5 @@ TEST(LoggerSDK, LogToAProcessor) // Log a sample log record to the processor auto r = std::shared_ptr(new opentelemetry::logs::LogRecord); r->name = "Test log"; - logger->log(r); + logger->Log(r); } From 2c1586f42ca0f473ba3ba8711bc89bdd0e8bbcb5 Mon Sep 17 00:00:00 2001 From: Seufert Date: Mon, 23 Nov 2020 19:08:25 -0700 Subject: [PATCH 03/21] Added GetName() to logger API, along with tests --- api/include/opentelemetry/logs/logger.h | 3 +-- api/include/opentelemetry/logs/noop.h | 1 + api/test/logs/logger_test.cc | 17 ++++++++--------- sdk/include/opentelemetry/sdk/logs/logger.h | 12 +++++++++++- sdk/src/logs/logger.cc | 10 ++++++++-- sdk/src/logs/logger_provider.cc | 2 +- sdk/test/logs/logger_sdk_test.cc | 10 ++++++++++ 7 files changed, 40 insertions(+), 15 deletions(-) diff --git a/api/include/opentelemetry/logs/logger.h b/api/include/opentelemetry/logs/logger.h index d0e1583d74..4b7b8c949c 100644 --- a/api/include/opentelemetry/logs/logger.h +++ b/api/include/opentelemetry/logs/logger.h @@ -39,8 +39,7 @@ class Logger virtual ~Logger() = default; /* Returns the name of the logger */ - // TODO: decide whether this is useful and/or should be kept, as this is not a method required in - // the specification. virtual nostd::string_view getName() = 0; + virtual nostd::string_view GetName() noexcept = 0; /** * Each of the following overloaded log(...) methods diff --git a/api/include/opentelemetry/logs/noop.h b/api/include/opentelemetry/logs/noop.h index cf41f63015..e831fbe07f 100644 --- a/api/include/opentelemetry/logs/noop.h +++ b/api/include/opentelemetry/logs/noop.h @@ -42,6 +42,7 @@ class NoopLogger final : public Logger NoopLogger() = default; void Log(nostd::shared_ptr record) noexcept override {} + nostd::string_view GetName() noexcept override { return "noop logger"; }; }; /** diff --git a/api/test/logs/logger_test.cc b/api/test/logs/logger_test.cc index 2667aa6b2a..815241dc0d 100644 --- a/api/test/logs/logger_test.cc +++ b/api/test/logs/logger_test.cc @@ -15,14 +15,15 @@ using opentelemetry::nostd::shared_ptr; using opentelemetry::nostd::span; using opentelemetry::nostd::string_view; -TEST(Logger, GetLoggerDefault) +TEST(Logger, GetLoggerDefaultNoop) { auto lp = Provider::GetLoggerProvider(); auto logger = lp->GetLogger("TestLogger"); EXPECT_NE(nullptr, logger); + EXPECT_EQ(logger->GetName(), "noop logger"); } -TEST(Logger, GetNoopLoggerName) +TEST(Logger, GetLoggerMethod) { auto lp = Provider::GetLoggerProvider(); @@ -57,6 +58,7 @@ TEST(Logger, LogMethod) class TestLogger : public Logger { void Log(opentelemetry::nostd::shared_ptr record) noexcept override {} + opentelemetry::nostd::string_view GetName() noexcept override { return "test logger"; }; }; // Define a basic LoggerProvider class that returns an instance of the logger class defined above @@ -75,17 +77,14 @@ class TestProvider : public LoggerProvider TEST(Logger, PushLoggerImplementation) { - // Push the new loggerprovider class into the global singleton + // Push the new loggerprovider class into the API auto test_provider = shared_ptr(new TestProvider()); Provider::SetLoggerProvider(test_provider); auto lp = Provider::GetLoggerProvider(); - // GetLogger(name, options) function + // Get a logger instance and check whether it's GetName() method returns + // "test logger" as defined in the custom implementation auto logger = lp->GetLogger("TestLogger"); - - // GetLogger(name, args) function - std::array sv{"string"}; - span args{sv}; - auto logger2 = lp->GetLogger("TestLogger2", args); + ASSERT_EQ("test logger", logger->GetName()); } diff --git a/sdk/include/opentelemetry/sdk/logs/logger.h b/sdk/include/opentelemetry/sdk/logs/logger.h index 48828bcf0e..6e9e9a68e3 100644 --- a/sdk/include/opentelemetry/sdk/logs/logger.h +++ b/sdk/include/opentelemetry/sdk/logs/logger.h @@ -39,9 +39,16 @@ class Logger final : public opentelemetry::logs::Logger public: /** * Initialize a new logger. + * @param name The name of this logger instance * @param logger_provider The logger provider that owns this logger. */ - explicit Logger(std::shared_ptr logger_provider) noexcept; + explicit Logger(opentelemetry::nostd::string_view name, + std::shared_ptr logger_provider) noexcept; + + /** + * Returns the name of this logger. + */ + opentelemetry::nostd::string_view GetName() noexcept override; /** * Writes a log record into the processor. @@ -54,6 +61,9 @@ class Logger final : public opentelemetry::logs::Logger // The logger provider of this Logger. Uses a weak_ptr to avoid cyclic dependancy issues the with // logger provider std::weak_ptr logger_provider_; + + // The name of this logger + opentelemetry::nostd::string_view logger_name_; }; } // namespace logs diff --git a/sdk/src/logs/logger.cc b/sdk/src/logs/logger.cc index 9556eb6b0e..a5195e0e07 100644 --- a/sdk/src/logs/logger.cc +++ b/sdk/src/logs/logger.cc @@ -21,10 +21,16 @@ namespace sdk { namespace logs { -Logger::Logger(std::shared_ptr logger_provider) noexcept - : logger_provider_(logger_provider) +Logger::Logger(opentelemetry::nostd::string_view name, + std::shared_ptr logger_provider) noexcept + : logger_name_(name), logger_provider_(logger_provider) {} +opentelemetry::nostd::string_view Logger::GetName() noexcept +{ + return logger_name_; +} + void Logger::Log(opentelemetry::nostd::shared_ptr record) noexcept { // If this logger does not have a processor, no need to create a log record diff --git a/sdk/src/logs/logger_provider.cc b/sdk/src/logs/logger_provider.cc index b040f9baab..6c978c3472 100644 --- a/sdk/src/logs/logger_provider.cc +++ b/sdk/src/logs/logger_provider.cc @@ -54,7 +54,7 @@ opentelemetry::nostd::shared_ptr LoggerProvider::Ge // If no logger with that name exists yet, create it and add it to the map of loggers opentelemetry::nostd::shared_ptr logger( - new Logger(this->shared_from_this())); + new Logger(name, this->shared_from_this())); loggers_[name.data()] = logger; return logger; } diff --git a/sdk/test/logs/logger_sdk_test.cc b/sdk/test/logs/logger_sdk_test.cc index 1265bfc183..0d1c9b94fc 100644 --- a/sdk/test/logs/logger_sdk_test.cc +++ b/sdk/test/logs/logger_sdk_test.cc @@ -20,6 +20,16 @@ using namespace opentelemetry::sdk::logs; +TEST(LoggerSDK, LoggerName) +{ + auto lp = std::shared_ptr(new LoggerProvider()); + auto logger1 = lp->GetLogger("logger1"); + auto logger2 = lp->GetLogger("logger2"); + + ASSERT_EQ("logger1", logger1->GetName()); + ASSERT_EQ("logger2", logger2->GetName()); +} + TEST(LoggerSDK, LogToNullProcessor) { // Confirm Logger::log() does not have undefined behavior From 350097df1d769bcda773b7d14e204aae31df091d Mon Sep 17 00:00:00 2001 From: Seufert Date: Tue, 24 Nov 2020 13:03:44 -0700 Subject: [PATCH 04/21] Processor accepts shared_ptr instead of unique_ptr --- sdk/include/opentelemetry/sdk/logs/processor.h | 2 +- sdk/src/logs/logger.cc | 6 ++---- sdk/test/logs/logger_provider_sdk_test.cc | 2 +- sdk/test/logs/logger_sdk_test.cc | 2 +- 4 files changed, 5 insertions(+), 7 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/logs/processor.h b/sdk/include/opentelemetry/sdk/logs/processor.h index d77b7a65d0..47b37baed0 100644 --- a/sdk/include/opentelemetry/sdk/logs/processor.h +++ b/sdk/include/opentelemetry/sdk/logs/processor.h @@ -34,7 +34,7 @@ class LogProcessor public: virtual ~LogProcessor() = default; - virtual void OnReceive(std::unique_ptr &&record) noexcept = 0; + virtual void OnReceive(nostd::shared_ptr record) noexcept = 0; virtual void ForceFlush( std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept = 0; diff --git a/sdk/src/logs/logger.cc b/sdk/src/logs/logger.cc index a5195e0e07..177be0f9b3 100644 --- a/sdk/src/logs/logger.cc +++ b/sdk/src/logs/logger.cc @@ -41,17 +41,15 @@ void Logger::Log(opentelemetry::nostd::shared_ptr( - new opentelemetry::logs::LogRecord(*record.get())); // TODO: Do not want to overwrite user-set timestamp if there already is one - // add a flag in the API to check if timestamp is set by user already before setting timestamp // Inject timestamp if none is set - record_pointer->timestamp = core::SystemTimestamp(std::chrono::system_clock::now()); + record->timestamp = core::SystemTimestamp(std::chrono::system_clock::now()); // TODO: inject traceid/spanid later // Send the log record to the processor - processor->OnReceive(std::move(record_pointer)); + processor->OnReceive(record); } } // namespace logs diff --git a/sdk/test/logs/logger_provider_sdk_test.cc b/sdk/test/logs/logger_provider_sdk_test.cc index a2a020c838..e1c7d9d421 100644 --- a/sdk/test/logs/logger_provider_sdk_test.cc +++ b/sdk/test/logs/logger_provider_sdk_test.cc @@ -69,7 +69,7 @@ TEST(LoggerProviderSDK, LoggerProviderLoggerArguments) class DummyProcessor : public LogProcessor { - void OnReceive(std::unique_ptr &&record) noexcept {} + void OnReceive(opentelemetry::nostd::shared_ptr record) noexcept {} void ForceFlush(std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept {} void Shutdown(std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept {} }; diff --git a/sdk/test/logs/logger_sdk_test.cc b/sdk/test/logs/logger_sdk_test.cc index 0d1c9b94fc..1beaa2610c 100644 --- a/sdk/test/logs/logger_sdk_test.cc +++ b/sdk/test/logs/logger_sdk_test.cc @@ -47,7 +47,7 @@ TEST(LoggerSDK, LogToNullProcessor) class DummyProcessor : public LogProcessor { - void OnReceive(std::unique_ptr &&record) noexcept {} + void OnReceive(opentelemetry::nostd::shared_ptr record) noexcept {} void ForceFlush(std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept {} void Shutdown(std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept {} }; From 7646ea2921fe70e5992dabc31fe7582b84298f1b Mon Sep 17 00:00:00 2001 From: Seufert Date: Tue, 24 Nov 2020 15:21:10 -0700 Subject: [PATCH 05/21] Added default values to the required log record fields --- api/include/opentelemetry/logs/log_record.h | 6 +++-- api/include/opentelemetry/logs/logger.h | 9 ++++++- api/test/logs/logger_test.cc | 26 +++++++++++++++++++++ 3 files changed, 38 insertions(+), 3 deletions(-) diff --git a/api/include/opentelemetry/logs/log_record.h b/api/include/opentelemetry/logs/log_record.h index c663c1c9c7..63452d1f3b 100644 --- a/api/include/opentelemetry/logs/log_record.h +++ b/api/include/opentelemetry/logs/log_record.h @@ -99,8 +99,10 @@ struct LogRecord **/ LogRecord() : resource(_nullKV), attributes(_nullKV) { - // TODO: in SDK, assign a default timestamp if not specified - name = ""; + // Assign default values + timestamp = core::SystemTimestamp(std::chrono::seconds(0)); + severity = Severity::kDefault; + } /* for ease of use; user can use this function to convert a map into a KeyValueIterable for the diff --git a/api/include/opentelemetry/logs/logger.h b/api/include/opentelemetry/logs/logger.h index 4b7b8c949c..c3a574e6b6 100644 --- a/api/include/opentelemetry/logs/logger.h +++ b/api/include/opentelemetry/logs/logger.h @@ -70,7 +70,14 @@ class Logger inline void Log(Severity severity, nostd::string_view message) noexcept { // TODO: set default timestamp later (not in API) - Log(severity, message, core::SystemTimestamp(std::chrono::system_clock::now())); + //Log(severity, message, core::SystemTimestamp(std::chrono::system_clock::now())); + + // creates a LogRecord object with given parameters, then calls log(LogRecord) + auto r = nostd::shared_ptr(new LogRecord); + r->severity = severity; + r->body = message; + + Log(r); } inline void Log(Severity severity, diff --git a/api/test/logs/logger_test.cc b/api/test/logs/logger_test.cc index 815241dc0d..e59096938e 100644 --- a/api/test/logs/logger_test.cc +++ b/api/test/logs/logger_test.cc @@ -54,6 +54,32 @@ TEST(Logger, LogMethod) logger->Log(r); } +TEST(Logger, LogRecordDefaults) +{ + auto r = opentelemetry::nostd::shared_ptr(new LogRecord); + + // Check that the timestamp is set to 0 by default + ASSERT_EQ(r->timestamp, opentelemetry::core::SystemTimestamp(std::chrono::seconds(0))); + + // Check that the severity is set to kDefault by default + ASSERT_EQ(r->severity, Severity::kDefault); + + // Check that trace_id is set to all zeros by default + char trace_buf[32]; + r->trace_id.ToLowerBase16(trace_buf); + ASSERT_EQ(std::string(trace_buf, sizeof(trace_buf)), "00000000000000000000000000000000"); + + // Check that span_id is set to all zeros by default + char span_buf[16]; + r->span_id.ToLowerBase16(span_buf); + ASSERT_EQ(std::string(span_buf, sizeof(span_buf)), "0000000000000000"); + + // Check that trace_flags is set to all zeros by default + char flags_buf[2]; + r->trace_flag.ToLowerBase16(flags_buf); + ASSERT_EQ(std::string(flags_buf, sizeof(flags_buf)), "00"); +} + // Define a basic Logger class class TestLogger : public Logger { From 25a3b1e84bc6a7134d4a67d8049d6e0d6b851d54 Mon Sep 17 00:00:00 2001 From: Seufert Date: Tue, 24 Nov 2020 17:07:45 -0700 Subject: [PATCH 06/21] Added timestamp injection and tests --- sdk/src/logs/logger.cc | 35 ++++++++++++++++++++++++++++---- sdk/test/logs/logger_sdk_test.cc | 30 +++++++++++---------------- 2 files changed, 43 insertions(+), 22 deletions(-) diff --git a/sdk/src/logs/logger.cc b/sdk/src/logs/logger.cc index 177be0f9b3..db17934030 100644 --- a/sdk/src/logs/logger.cc +++ b/sdk/src/logs/logger.cc @@ -41,11 +41,38 @@ void Logger::Log(opentelemetry::nostd::shared_ptrtimestamp = core::SystemTimestamp(std::chrono::system_clock::now()); + // Inject values into record if not user specified + // Timestamp + if (record->timestamp == opentelemetry::core::SystemTimestamp(std::chrono::seconds(0))) + record->timestamp = core::SystemTimestamp(std::chrono::system_clock::now()); + + // Traceid + char trace_buf[32]; + record->trace_id.ToLowerBase16(trace_buf); + if (std::string(trace_buf, sizeof(trace_buf)), "00000000000000000000000000000000") + { + //TODO + } + + // Spanid + char span_buf[16]; + record->span_id.ToLowerBase16(span_buf); + if (std::string(span_buf, sizeof(span_buf)), "0000000000000000") + { + //TODO + } + + // Traceflag + char flag_buf[2]; + record->trace_flag.ToLowerBase16(flag_buf); + if (std::string(flag_buf, sizeof(flag_buf)), "00") + { + //TODO + } + + // Inject logger name into record + // TODO: inject traceid/spanid later // Send the log record to the processor diff --git a/sdk/test/logs/logger_sdk_test.cc b/sdk/test/logs/logger_sdk_test.cc index 1beaa2610c..3219bbac92 100644 --- a/sdk/test/logs/logger_sdk_test.cc +++ b/sdk/test/logs/logger_sdk_test.cc @@ -52,30 +52,24 @@ class DummyProcessor : public LogProcessor void Shutdown(std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept {} }; -TEST(LoggerSDK, LogToAProcessor) +TEST(LoggerSDK, DefaultValueInjection) { - // Create an API LoggerProvider and logger - auto api_lp = std::shared_ptr(new LoggerProvider()); - auto logger = api_lp->GetLogger("logger"); - - // Cast the API LoggerProvider to an SDK Logger Provider and assert that it is still the same - // LoggerProvider by checking that getting a logger with the same name as the previously defined - // logger is the same instance - auto lp = static_cast(api_lp.get()); - auto logger2 = lp->GetLogger("logger"); - ASSERT_EQ(logger, logger2); - - // Set a processor for the LoggerProvider + // In order to test value injection, the processor must not be nullptr + // A DummyProcessor was created above to satisfy this requirement std::shared_ptr processor = std::shared_ptr(new DummyProcessor()); + auto lp = std::shared_ptr(new LoggerProvider()); lp->SetProcessor(processor); - ASSERT_EQ(processor, lp->GetProcessor()); - - // Should later introduce a way to assert that - // the logger's processor is the same as "proc" - // and that the logger's processor is the same as lp's processor + auto logger = lp->GetLogger("Logger1"); // Log a sample log record to the processor auto r = std::shared_ptr(new opentelemetry::logs::LogRecord); r->name = "Test log"; logger->Log(r); + + // Check that the log record has injected values + + // Timestamp shouldn't equal 0 + ASSERT_NE(r->timestamp, opentelemetry::core::SystemTimestamp(std::chrono::seconds(0))); + + // TODO: Add checks for traceid, spanid, and traceflags once it gets added } From dc8f9b936a9c9da7c13a18409819b8bcc31a48d5 Mon Sep 17 00:00:00 2001 From: Seufert Date: Wed, 25 Nov 2020 11:58:22 -0700 Subject: [PATCH 07/21] Return noop logger when logger limit reached, formatting --- api/include/opentelemetry/logs/log_record.h | 3 +-- api/include/opentelemetry/logs/logger.h | 10 +++++----- api/test/logs/logger_test.cc | 2 +- .../opentelemetry/sdk/logs/logger_provider.h | 7 +++++-- sdk/src/logs/logger.cc | 16 ++++++++-------- sdk/src/logs/logger_provider.cc | 19 ++++++++----------- sdk/test/logs/logger_provider_sdk_test.cc | 19 ++++++++++++++++++- sdk/test/logs/logger_sdk_test.cc | 5 +++-- 8 files changed, 49 insertions(+), 32 deletions(-) diff --git a/api/include/opentelemetry/logs/log_record.h b/api/include/opentelemetry/logs/log_record.h index 63452d1f3b..3882f55ef5 100644 --- a/api/include/opentelemetry/logs/log_record.h +++ b/api/include/opentelemetry/logs/log_record.h @@ -101,8 +101,7 @@ struct LogRecord { // Assign default values timestamp = core::SystemTimestamp(std::chrono::seconds(0)); - severity = Severity::kDefault; - + severity = Severity::kDefault; } /* for ease of use; user can use this function to convert a map into a KeyValueIterable for the diff --git a/api/include/opentelemetry/logs/logger.h b/api/include/opentelemetry/logs/logger.h index c3a574e6b6..5eebddc7bc 100644 --- a/api/include/opentelemetry/logs/logger.h +++ b/api/include/opentelemetry/logs/logger.h @@ -70,13 +70,13 @@ class Logger inline void Log(Severity severity, nostd::string_view message) noexcept { // TODO: set default timestamp later (not in API) - //Log(severity, message, core::SystemTimestamp(std::chrono::system_clock::now())); + // Log(severity, message, core::SystemTimestamp(std::chrono::system_clock::now())); // creates a LogRecord object with given parameters, then calls log(LogRecord) - auto r = nostd::shared_ptr(new LogRecord); - r->severity = severity; - r->body = message; - + auto r = nostd::shared_ptr(new LogRecord); + r->severity = severity; + r->body = message; + Log(r); } diff --git a/api/test/logs/logger_test.cc b/api/test/logs/logger_test.cc index e59096938e..9d1d5fd5f6 100644 --- a/api/test/logs/logger_test.cc +++ b/api/test/logs/logger_test.cc @@ -56,7 +56,7 @@ TEST(Logger, LogMethod) TEST(Logger, LogRecordDefaults) { - auto r = opentelemetry::nostd::shared_ptr(new LogRecord); + auto r = opentelemetry::nostd::shared_ptr(new LogRecord); // Check that the timestamp is set to 0 by default ASSERT_EQ(r->timestamp, opentelemetry::core::SystemTimestamp(std::chrono::seconds(0))); diff --git a/sdk/include/opentelemetry/sdk/logs/logger_provider.h b/sdk/include/opentelemetry/sdk/logs/logger_provider.h index fdba956624..e6c4f23073 100644 --- a/sdk/include/opentelemetry/sdk/logs/logger_provider.h +++ b/sdk/include/opentelemetry/sdk/logs/logger_provider.h @@ -29,8 +29,8 @@ #include "opentelemetry/sdk/logs/processor.h" // Define the maximum number of loggers that are allowed to be registered to the loggerprovider. -// TODO: Add link to logging spec once this is added to it -#define MAX_LOGGER_COUNT 100 +// References spec issue https://github.com/open-telemetry/opentelemetry-specification/issues/1259 +#define OTEL_MAX_LOGGER_COUNT 1000 OPENTELEMETRY_BEGIN_NAMESPACE namespace sdk @@ -95,6 +95,9 @@ class LoggerProvider final : public opentelemetry::logs::LoggerProvider, // A mutex that ensures only one thread is using the map of loggers std::mutex mu_; + + // A noop logger that is returned by GetLogger() when OTEL_MAX_LOGGER_COUNT reached + opentelemetry::nostd::shared_ptr noop_logger_; }; } // namespace logs } // namespace sdk diff --git a/sdk/src/logs/logger.cc b/sdk/src/logs/logger.cc index db17934030..3b7b2fb7a3 100644 --- a/sdk/src/logs/logger.cc +++ b/sdk/src/logs/logger.cc @@ -51,28 +51,28 @@ void Logger::Log(opentelemetry::nostd::shared_ptrtrace_id.ToLowerBase16(trace_buf); if (std::string(trace_buf, sizeof(trace_buf)), "00000000000000000000000000000000") - { - //TODO + { + // TODO } // Spanid char span_buf[16]; record->span_id.ToLowerBase16(span_buf); if (std::string(span_buf, sizeof(span_buf)), "0000000000000000") - { - //TODO + { + // TODO } // Traceflag char flag_buf[2]; record->trace_flag.ToLowerBase16(flag_buf); if (std::string(flag_buf, sizeof(flag_buf)), "00") - { - //TODO + { + // TODO } - + // Inject logger name into record - + // TODO: inject traceid/spanid later // Send the log record to the processor diff --git a/sdk/src/logs/logger_provider.cc b/sdk/src/logs/logger_provider.cc index 6c978c3472..697619ba56 100644 --- a/sdk/src/logs/logger_provider.cc +++ b/sdk/src/logs/logger_provider.cc @@ -22,7 +22,11 @@ namespace sdk namespace logs { -LoggerProvider::LoggerProvider() noexcept : processor_{nullptr} {} +LoggerProvider::LoggerProvider() noexcept + : processor_{nullptr}, + noop_logger_{ + nostd::shared_ptr(new opentelemetry::logs::NoopLogger)} +{} opentelemetry::nostd::shared_ptr LoggerProvider::GetLogger( opentelemetry::nostd::string_view name, @@ -39,20 +43,13 @@ opentelemetry::nostd::shared_ptr LoggerProvider::Ge } // Check if creating a new logger would exceed the max number of loggers - // TODO: Remove the noexcept from the API's and SDK's GetLogger(~) - /* - if (loggers_.size() > MAX_LOGGER_COUNT) + if (loggers_.size() >= OTEL_MAX_LOGGER_COUNT) { -#if __EXCEPTIONS - throw std::length_error("Number of loggers exceeds max count"); -#else - std::terminate(); -#endif + // Return a noop logger + return noop_logger_; } - */ // If no logger with that name exists yet, create it and add it to the map of loggers - opentelemetry::nostd::shared_ptr logger( new Logger(name, this->shared_from_this())); loggers_[name.data()] = logger; diff --git a/sdk/test/logs/logger_provider_sdk_test.cc b/sdk/test/logs/logger_provider_sdk_test.cc index e1c7d9d421..1a01471234 100644 --- a/sdk/test/logs/logger_provider_sdk_test.cc +++ b/sdk/test/logs/logger_provider_sdk_test.cc @@ -69,7 +69,8 @@ TEST(LoggerProviderSDK, LoggerProviderLoggerArguments) class DummyProcessor : public LogProcessor { - void OnReceive(opentelemetry::nostd::shared_ptr record) noexcept {} + void OnReceive(opentelemetry::nostd::shared_ptr record) noexcept + {} void ForceFlush(std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept {} void Shutdown(std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept {} }; @@ -85,3 +86,19 @@ TEST(LoggerProviderSDK, GetAndSetProcessor) lp.SetProcessor(proc2); ASSERT_EQ(proc2, lp.GetProcessor()); } + +TEST(LoggerProviderSDK, LoggerLimit) +{ + auto lp = std::shared_ptr(new LoggerProvider()); + + // Create as many loggers as the logger provider allows + for (int i = 0; i < OTEL_MAX_LOGGER_COUNT; i++) + { + lp->GetLogger(std::to_string(i)); + } + + // Creating a new logger will return a noop logger + auto logger = lp->GetLogger("Another logger"); + opentelemetry::logs::NoopLogger nooplogger; + ASSERT_EQ(logger->GetName(), nooplogger.GetName()); +} \ No newline at end of file diff --git a/sdk/test/logs/logger_sdk_test.cc b/sdk/test/logs/logger_sdk_test.cc index 3219bbac92..e2193f0df7 100644 --- a/sdk/test/logs/logger_sdk_test.cc +++ b/sdk/test/logs/logger_sdk_test.cc @@ -47,7 +47,8 @@ TEST(LoggerSDK, LogToNullProcessor) class DummyProcessor : public LogProcessor { - void OnReceive(opentelemetry::nostd::shared_ptr record) noexcept {} + void OnReceive(opentelemetry::nostd::shared_ptr record) noexcept + {} void ForceFlush(std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept {} void Shutdown(std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept {} }; @@ -57,7 +58,7 @@ TEST(LoggerSDK, DefaultValueInjection) // In order to test value injection, the processor must not be nullptr // A DummyProcessor was created above to satisfy this requirement std::shared_ptr processor = std::shared_ptr(new DummyProcessor()); - auto lp = std::shared_ptr(new LoggerProvider()); + auto lp = std::shared_ptr(new LoggerProvider()); lp->SetProcessor(processor); auto logger = lp->GetLogger("Logger1"); From 0ee12c6043aa18bef92c3332d7fb222e59d567c4 Mon Sep 17 00:00:00 2001 From: Karen Xu Date: Mon, 30 Nov 2020 20:51:07 -0500 Subject: [PATCH 08/21] Rename trace_flag to trace_flags in LogRecord --- api/include/opentelemetry/logs/log_record.h | 5 ++--- api/test/logs/logger_test.cc | 2 +- sdk/src/logs/logger.cc | 2 +- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/api/include/opentelemetry/logs/log_record.h b/api/include/opentelemetry/logs/log_record.h index 3882f55ef5..01982df77d 100644 --- a/api/include/opentelemetry/logs/log_record.h +++ b/api/include/opentelemetry/logs/log_record.h @@ -82,9 +82,8 @@ struct LogRecord core::SystemTimestamp timestamp; // uint64 nanoseconds since Unix epoch trace::TraceId trace_id; // byte sequence trace::SpanId span_id; // byte sequence - trace::TraceFlags trace_flag; // byte - Severity severity; // Severity enum that combines severity_text and severity_number in the - // LogDataModel (can separate in SDK) + trace::TraceFlags trace_flags; // byte + Severity severity; // Severity enum that combines severity_text and severity_number // other fields that will not be set by default nostd::string_view name; // string diff --git a/api/test/logs/logger_test.cc b/api/test/logs/logger_test.cc index 9d1d5fd5f6..ddf97209a4 100644 --- a/api/test/logs/logger_test.cc +++ b/api/test/logs/logger_test.cc @@ -76,7 +76,7 @@ TEST(Logger, LogRecordDefaults) // Check that trace_flags is set to all zeros by default char flags_buf[2]; - r->trace_flag.ToLowerBase16(flags_buf); + r->trace_flags.ToLowerBase16(flags_buf); ASSERT_EQ(std::string(flags_buf, sizeof(flags_buf)), "00"); } diff --git a/sdk/src/logs/logger.cc b/sdk/src/logs/logger.cc index 3b7b2fb7a3..080fc695ed 100644 --- a/sdk/src/logs/logger.cc +++ b/sdk/src/logs/logger.cc @@ -65,7 +65,7 @@ void Logger::Log(opentelemetry::nostd::shared_ptrtrace_flag.ToLowerBase16(flag_buf); + record->trace_flags.ToLowerBase16(flag_buf); if (std::string(flag_buf, sizeof(flag_buf)), "00") { // TODO From 595ab5b46787102f3078a72b1c76cd43645ed611 Mon Sep 17 00:00:00 2001 From: Karen Xu Date: Mon, 30 Nov 2020 20:58:11 -0500 Subject: [PATCH 09/21] Set default traceid, spanid, traceflags in LogRecord.h --- api/include/opentelemetry/logs/log_record.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/api/include/opentelemetry/logs/log_record.h b/api/include/opentelemetry/logs/log_record.h index 01982df77d..f150848344 100644 --- a/api/include/opentelemetry/logs/log_record.h +++ b/api/include/opentelemetry/logs/log_record.h @@ -101,6 +101,9 @@ struct LogRecord // Assign default values timestamp = core::SystemTimestamp(std::chrono::seconds(0)); severity = Severity::kDefault; + trace_id = trace::TraceId(); + span_id = trace::SpanId(); + trace_flags = trace::TraceFlags(); } /* for ease of use; user can use this function to convert a map into a KeyValueIterable for the From 6083373d2f474e5b9b8b50899e0e8e933d083da9 Mon Sep 17 00:00:00 2001 From: Karen Xu Date: Mon, 30 Nov 2020 21:14:27 -0500 Subject: [PATCH 10/21] Inject spancontext (traceid, spanid, traceflags) --- sdk/src/logs/BUILD | 1 + sdk/src/logs/CMakeLists.txt | 2 +- sdk/src/logs/logger.cc | 15 +++++++++++---- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/sdk/src/logs/BUILD b/sdk/src/logs/BUILD index c1a0542fa1..4829b3838f 100644 --- a/sdk/src/logs/BUILD +++ b/sdk/src/logs/BUILD @@ -22,5 +22,6 @@ cc_library( deps = [ "//api", "//sdk:headers", + "//sdk/src/trace", ], ) diff --git a/sdk/src/logs/CMakeLists.txt b/sdk/src/logs/CMakeLists.txt index 22260f059d..88380775ca 100644 --- a/sdk/src/logs/CMakeLists.txt +++ b/sdk/src/logs/CMakeLists.txt @@ -1,3 +1,3 @@ add_library(opentelemetry_logs logger_provider.cc logger.cc) -target_link_libraries(opentelemetry_logs opentelemetry_common) +target_link_libraries(opentelemetry_logs opentelemetry_common opentelemetry_trace) diff --git a/sdk/src/logs/logger.cc b/sdk/src/logs/logger.cc index 080fc695ed..0711761747 100644 --- a/sdk/src/logs/logger.cc +++ b/sdk/src/logs/logger.cc @@ -15,6 +15,8 @@ */ #include "opentelemetry/sdk/logs/logger.h" +#include "opentelemetry/trace/provider.h" +#include "opentelemetry/sdk/trace/span_data.h" OPENTELEMETRY_BEGIN_NAMESPACE namespace sdk @@ -47,12 +49,17 @@ void Logger::Log(opentelemetry::nostd::shared_ptrtimestamp == opentelemetry::core::SystemTimestamp(std::chrono::seconds(0))) record->timestamp = core::SystemTimestamp(std::chrono::system_clock::now()); - // Traceid + + // Inject traceid/spanid (if none is set) + auto provider = opentelemetry::trace::Provider::GetTracerProvider(); + auto tracer = provider->GetTracer("foo_library"); + auto span_context = tracer->GetCurrentSpan()->GetContext(); + char trace_buf[32]; record->trace_id.ToLowerBase16(trace_buf); if (std::string(trace_buf, sizeof(trace_buf)), "00000000000000000000000000000000") { - // TODO + record->trace_id = span_context.trace_id(); } // Spanid @@ -60,7 +67,7 @@ void Logger::Log(opentelemetry::nostd::shared_ptrspan_id.ToLowerBase16(span_buf); if (std::string(span_buf, sizeof(span_buf)), "0000000000000000") { - // TODO + record->span_id = span_context.span_id(); } // Traceflag @@ -68,7 +75,7 @@ void Logger::Log(opentelemetry::nostd::shared_ptrtrace_flags.ToLowerBase16(flag_buf); if (std::string(flag_buf, sizeof(flag_buf)), "00") { - // TODO + record->trace_flags = span_context.trace_flags(); } // Inject logger name into record From 4c50ef2438178b7ee42b0bfdc87a681aba35a2c4 Mon Sep 17 00:00:00 2001 From: Seufert Date: Tue, 1 Dec 2020 14:02:51 -0700 Subject: [PATCH 11/21] Simplified logger context injection, added unit tests --- sdk/src/logs/logger.cc | 15 ++++---------- sdk/test/logs/BUILD | 2 ++ sdk/test/logs/CMakeLists.txt | 7 +++++-- sdk/test/logs/logger_sdk_test.cc | 35 ++++++++++++++++++++++++++++++-- 4 files changed, 44 insertions(+), 15 deletions(-) diff --git a/sdk/src/logs/logger.cc b/sdk/src/logs/logger.cc index 0711761747..9945e27e1f 100644 --- a/sdk/src/logs/logger.cc +++ b/sdk/src/logs/logger.cc @@ -55,33 +55,26 @@ void Logger::Log(opentelemetry::nostd::shared_ptrGetTracer("foo_library"); auto span_context = tracer->GetCurrentSpan()->GetContext(); - char trace_buf[32]; - record->trace_id.ToLowerBase16(trace_buf); - if (std::string(trace_buf, sizeof(trace_buf)), "00000000000000000000000000000000") + // Traceid + if (!record->trace_id.IsValid()) { record->trace_id = span_context.trace_id(); } // Spanid - char span_buf[16]; - record->span_id.ToLowerBase16(span_buf); - if (std::string(span_buf, sizeof(span_buf)), "0000000000000000") + if (!record->span_id.IsValid()) { record->span_id = span_context.span_id(); } // Traceflag - char flag_buf[2]; - record->trace_flags.ToLowerBase16(flag_buf); - if (std::string(flag_buf, sizeof(flag_buf)), "00") + if (!record->trace_flags.IsSampled()) { record->trace_flags = span_context.trace_flags(); } // Inject logger name into record - // TODO: inject traceid/spanid later - // Send the log record to the processor processor->OnReceive(record); } diff --git a/sdk/test/logs/BUILD b/sdk/test/logs/BUILD index b58eac0eee..e5cd20ae7c 100644 --- a/sdk/test/logs/BUILD +++ b/sdk/test/logs/BUILD @@ -17,6 +17,8 @@ cc_test( ], deps = [ "//sdk/src/logs", + "//sdk/src/trace", + "//exporters/memory:in_memory_span_exporter", "@com_google_googletest//:gtest_main", ], ) diff --git a/sdk/test/logs/CMakeLists.txt b/sdk/test/logs/CMakeLists.txt index 87f7ae9a96..5de4ae2ee1 100644 --- a/sdk/test/logs/CMakeLists.txt +++ b/sdk/test/logs/CMakeLists.txt @@ -1,6 +1,9 @@ -foreach(testname logger_provider_sdk_test logger_sdk_test) +foreach( + testname + logger_provider_sdk_test + logger_sdk_test) add_executable(${testname} "${testname}.cc") target_link_libraries(${testname} ${GTEST_BOTH_LIBRARIES} - ${CMAKE_THREAD_LIBS_INIT} opentelemetry_logs) + ${CMAKE_THREAD_LIBS_INIT} opentelemetry_logs opentelemetry_exporter_in_memory) gtest_add_tests(TARGET ${testname} TEST_PREFIX logs. TEST_LIST ${testname}) endforeach() diff --git a/sdk/test/logs/logger_sdk_test.cc b/sdk/test/logs/logger_sdk_test.cc index e2193f0df7..1e285ec544 100644 --- a/sdk/test/logs/logger_sdk_test.cc +++ b/sdk/test/logs/logger_sdk_test.cc @@ -15,10 +15,15 @@ */ #include "opentelemetry/sdk/logs/logger.h" +#include "opentelemetry/sdk/trace/tracer_provider.h" +#include "opentelemetry/trace/provider.h" +#include "opentelemetry/exporters/memory/in_memory_span_exporter.h" +#include "opentelemetry/sdk/trace/simple_processor.h" #include using namespace opentelemetry::sdk::logs; +namespace sdktrace = opentelemetry::sdk::trace; TEST(LoggerSDK, LoggerName) { @@ -68,9 +73,35 @@ TEST(LoggerSDK, DefaultValueInjection) logger->Log(r); // Check that the log record has injected values - // Timestamp shouldn't equal 0 ASSERT_NE(r->timestamp, opentelemetry::core::SystemTimestamp(std::chrono::seconds(0))); - // TODO: Add checks for traceid, spanid, and traceflags once it gets added + //Check that the traceid, spanid, and traceflags are not valid since there is no trace context + ASSERT_FALSE(r->trace_id.IsValid()); + ASSERT_FALSE(r->span_id.IsValid()); + ASSERT_FALSE(r->trace_flags.IsSampled()); + + // To test traceid/spanid/traceflags injection, initialize the tracing pipeline + std::unique_ptr trace_exporter(new opentelemetry::exporter::memory::InMemorySpanExporter()); + auto trace_processor = std::make_shared(std::move(trace_exporter)); + auto trace_provider = opentelemetry::nostd::shared_ptr( + new sdktrace::TracerProvider(trace_processor)); + opentelemetry::trace::Provider::SetTracerProvider(trace_provider); + + //Create a tracer and start a span for span context + auto tracer = trace_provider->GetTracer("foo_library"); + auto span_first = tracer->StartSpan("span 1"); + auto scope_first = tracer->WithActiveSpan(span_first); + auto span_second = tracer->StartSpan("span 2"); + + // Log a sample log record to the processor + auto r2 = std::shared_ptr(new opentelemetry::logs::LogRecord); + r2->name = "Test log"; + logger->Log(r2); + + //Check that the traceid, spanid, and traceflags were injected from the span context properly + auto span_context = tracer->GetCurrentSpan()->GetContext(); + ASSERT_EQ(r2->trace_id, span_context.trace_id()); + ASSERT_EQ(r2->span_id, span_context.span_id()); + ASSERT_EQ(r2->trace_flags, span_context.trace_flags()); } From 18b921afa45257481a319fb952b4c613e008167c Mon Sep 17 00:00:00 2001 From: Seufert Date: Tue, 1 Dec 2020 14:53:47 -0700 Subject: [PATCH 12/21] Changed log(LogRecord) back from log(shared_ptr) --- api/include/opentelemetry/logs/logger.h | 31 ++++++++++----------- api/include/opentelemetry/logs/noop.h | 2 +- api/test/logs/logger_test.cc | 20 ++++++------- sdk/include/opentelemetry/sdk/logs/logger.h | 3 +- sdk/src/logs/logger.cc | 23 ++++++++------- sdk/test/logs/logger_sdk_test.cc | 28 ++++++++++--------- 6 files changed, 54 insertions(+), 53 deletions(-) diff --git a/api/include/opentelemetry/logs/logger.h b/api/include/opentelemetry/logs/logger.h index 5eebddc7bc..0138d0348c 100644 --- a/api/include/opentelemetry/logs/logger.h +++ b/api/include/opentelemetry/logs/logger.h @@ -58,7 +58,7 @@ class Logger * A default LogRecord that will be assigned if no parameters are passed to Logger's .log() method * which should at minimum assign the trace_id, span_id, and timestamp */ - virtual void Log(nostd::shared_ptr record) noexcept = 0; + virtual void Log(LogRecord &record) noexcept = 0; /** Overloaded methods for unstructured logging **/ inline void Log(nostd::string_view message) noexcept @@ -69,13 +69,10 @@ class Logger inline void Log(Severity severity, nostd::string_view message) noexcept { - // TODO: set default timestamp later (not in API) - // Log(severity, message, core::SystemTimestamp(std::chrono::system_clock::now())); - - // creates a LogRecord object with given parameters, then calls log(LogRecord) - auto r = nostd::shared_ptr(new LogRecord); - r->severity = severity; - r->body = message; + // creates a LogRecord object with given parameters, then calls Log(LogRecord) + LogRecord r; + r.severity = severity; + r.body = message; Log(r); } @@ -84,11 +81,11 @@ class Logger nostd::string_view message, core::SystemTimestamp time) noexcept { - // creates a LogRecord object with given parameters, then calls log(LogRecord) - auto r = nostd::shared_ptr(new LogRecord); - r->severity = severity; - r->body = message; - r->timestamp = time; + // creates a LogRecord object with given parameters, then calls Log(LogRecord) + LogRecord r; + r.severity = severity; + r.body = message; + r.timestamp = time; Log(r); } @@ -110,10 +107,10 @@ class Logger const common::KeyValueIterable &attributes) noexcept { // creates a LogRecord object with given parameters, then calls log(LogRecord) - auto r = nostd::shared_ptr(new LogRecord); - r->severity = severity; - r->name = name; - r->attributes = attributes; + LogRecord r; + r.severity = severity; + r.name = name; + r.attributes = attributes; Log(r); } diff --git a/api/include/opentelemetry/logs/noop.h b/api/include/opentelemetry/logs/noop.h index e831fbe07f..4841320d52 100644 --- a/api/include/opentelemetry/logs/noop.h +++ b/api/include/opentelemetry/logs/noop.h @@ -41,7 +41,7 @@ class NoopLogger final : public Logger public: NoopLogger() = default; - void Log(nostd::shared_ptr record) noexcept override {} + void Log(LogRecord& record) noexcept override {} nostd::string_view GetName() noexcept override { return "noop logger"; }; }; diff --git a/api/test/logs/logger_test.cc b/api/test/logs/logger_test.cc index ddf97209a4..0e3a420df1 100644 --- a/api/test/logs/logger_test.cc +++ b/api/test/logs/logger_test.cc @@ -48,42 +48,42 @@ TEST(Logger, LogMethod) logger->Log(Severity::kError, "Error message"); // Test log(LogRecord) - auto r = opentelemetry::nostd::shared_ptr(new LogRecord); - r->name = "Log Record"; - r->severity = Severity::kInfo; + LogRecord r; + r.name = "Log Record"; + r.severity = Severity::kInfo; logger->Log(r); } TEST(Logger, LogRecordDefaults) { - auto r = opentelemetry::nostd::shared_ptr(new LogRecord); + LogRecord r; // Check that the timestamp is set to 0 by default - ASSERT_EQ(r->timestamp, opentelemetry::core::SystemTimestamp(std::chrono::seconds(0))); + ASSERT_EQ(r.timestamp, opentelemetry::core::SystemTimestamp(std::chrono::seconds(0))); // Check that the severity is set to kDefault by default - ASSERT_EQ(r->severity, Severity::kDefault); + ASSERT_EQ(r.severity, Severity::kDefault); // Check that trace_id is set to all zeros by default char trace_buf[32]; - r->trace_id.ToLowerBase16(trace_buf); + r.trace_id.ToLowerBase16(trace_buf); ASSERT_EQ(std::string(trace_buf, sizeof(trace_buf)), "00000000000000000000000000000000"); // Check that span_id is set to all zeros by default char span_buf[16]; - r->span_id.ToLowerBase16(span_buf); + r.span_id.ToLowerBase16(span_buf); ASSERT_EQ(std::string(span_buf, sizeof(span_buf)), "0000000000000000"); // Check that trace_flags is set to all zeros by default char flags_buf[2]; - r->trace_flags.ToLowerBase16(flags_buf); + r.trace_flags.ToLowerBase16(flags_buf); ASSERT_EQ(std::string(flags_buf, sizeof(flags_buf)), "00"); } // Define a basic Logger class class TestLogger : public Logger { - void Log(opentelemetry::nostd::shared_ptr record) noexcept override {} + void Log(LogRecord &record) noexcept override {} opentelemetry::nostd::string_view GetName() noexcept override { return "test logger"; }; }; diff --git a/sdk/include/opentelemetry/sdk/logs/logger.h b/sdk/include/opentelemetry/sdk/logs/logger.h index 6e9e9a68e3..b36abd9f79 100644 --- a/sdk/include/opentelemetry/sdk/logs/logger.h +++ b/sdk/include/opentelemetry/sdk/logs/logger.h @@ -54,8 +54,7 @@ class Logger final : public opentelemetry::logs::Logger * Writes a log record into the processor. * @param record The record to write into the processor. */ - void Log( - opentelemetry::nostd::shared_ptr record) noexcept override; + void Log(opentelemetry::logs::LogRecord &record) noexcept override; private: // The logger provider of this Logger. Uses a weak_ptr to avoid cyclic dependancy issues the with diff --git a/sdk/src/logs/logger.cc b/sdk/src/logs/logger.cc index 9945e27e1f..953bb1e2a5 100644 --- a/sdk/src/logs/logger.cc +++ b/sdk/src/logs/logger.cc @@ -33,7 +33,7 @@ opentelemetry::nostd::string_view Logger::GetName() noexcept return logger_name_; } -void Logger::Log(opentelemetry::nostd::shared_ptr record) noexcept +void Logger::Log(opentelemetry::logs::LogRecord &record) noexcept { // If this logger does not have a processor, no need to create a log record auto processor = logger_provider_.lock()->GetProcessor(); @@ -44,10 +44,13 @@ void Logger::Log(opentelemetry::nostd::shared_ptr(new opentelemetry::logs::LogRecord(record)); + // Inject values into record if not user specified // Timestamp - if (record->timestamp == opentelemetry::core::SystemTimestamp(std::chrono::seconds(0))) - record->timestamp = core::SystemTimestamp(std::chrono::system_clock::now()); + if (r->timestamp == opentelemetry::core::SystemTimestamp(std::chrono::seconds(0))) + r->timestamp = core::SystemTimestamp(std::chrono::system_clock::now()); // Inject traceid/spanid (if none is set) @@ -56,27 +59,27 @@ void Logger::Log(opentelemetry::nostd::shared_ptrGetCurrentSpan()->GetContext(); // Traceid - if (!record->trace_id.IsValid()) + if (!r->trace_id.IsValid()) { - record->trace_id = span_context.trace_id(); + r->trace_id = span_context.trace_id(); } // Spanid - if (!record->span_id.IsValid()) + if (!r->span_id.IsValid()) { - record->span_id = span_context.span_id(); + r->span_id = span_context.span_id(); } // Traceflag - if (!record->trace_flags.IsSampled()) + if (!r->trace_flags.IsSampled()) { - record->trace_flags = span_context.trace_flags(); + r->trace_flags = span_context.trace_flags(); } // Inject logger name into record // Send the log record to the processor - processor->OnReceive(record); + processor->OnReceive(r); } } // namespace logs diff --git a/sdk/test/logs/logger_sdk_test.cc b/sdk/test/logs/logger_sdk_test.cc index 1e285ec544..831095e3e2 100644 --- a/sdk/test/logs/logger_sdk_test.cc +++ b/sdk/test/logs/logger_sdk_test.cc @@ -45,8 +45,8 @@ TEST(LoggerSDK, LogToNullProcessor) auto logger = lp->GetLogger("logger"); // Log a sample log record to a nullptr processor - auto r = std::shared_ptr(new opentelemetry::logs::LogRecord); - r->name = "Test log"; + opentelemetry::logs::LogRecord r; + r.name = "Test log"; logger->Log(r); } @@ -58,6 +58,7 @@ class DummyProcessor : public LogProcessor void Shutdown(std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept {} }; +/* TEST(LoggerSDK, DefaultValueInjection) { // In order to test value injection, the processor must not be nullptr @@ -68,18 +69,18 @@ TEST(LoggerSDK, DefaultValueInjection) auto logger = lp->GetLogger("Logger1"); // Log a sample log record to the processor - auto r = std::shared_ptr(new opentelemetry::logs::LogRecord); - r->name = "Test log"; + opentelemetry::logs::LogRecord r; + r.name = "Test log"; logger->Log(r); // Check that the log record has injected values // Timestamp shouldn't equal 0 - ASSERT_NE(r->timestamp, opentelemetry::core::SystemTimestamp(std::chrono::seconds(0))); + ASSERT_NE(r.timestamp, opentelemetry::core::SystemTimestamp(std::chrono::seconds(0))); //Check that the traceid, spanid, and traceflags are not valid since there is no trace context - ASSERT_FALSE(r->trace_id.IsValid()); - ASSERT_FALSE(r->span_id.IsValid()); - ASSERT_FALSE(r->trace_flags.IsSampled()); + ASSERT_FALSE(r.trace_id.IsValid()); + ASSERT_FALSE(r.span_id.IsValid()); + ASSERT_FALSE(r.trace_flags.IsSampled()); // To test traceid/spanid/traceflags injection, initialize the tracing pipeline std::unique_ptr trace_exporter(new opentelemetry::exporter::memory::InMemorySpanExporter()); @@ -95,13 +96,14 @@ TEST(LoggerSDK, DefaultValueInjection) auto span_second = tracer->StartSpan("span 2"); // Log a sample log record to the processor - auto r2 = std::shared_ptr(new opentelemetry::logs::LogRecord); - r2->name = "Test log"; + opentelemetry::logs::LogRecord r2; + r2.name = "Test log"; logger->Log(r2); //Check that the traceid, spanid, and traceflags were injected from the span context properly auto span_context = tracer->GetCurrentSpan()->GetContext(); - ASSERT_EQ(r2->trace_id, span_context.trace_id()); - ASSERT_EQ(r2->span_id, span_context.span_id()); - ASSERT_EQ(r2->trace_flags, span_context.trace_flags()); + ASSERT_EQ(r2.trace_id, span_context.trace_id()); + ASSERT_EQ(r2.span_id, span_context.span_id()); + ASSERT_EQ(r2.trace_flags, span_context.trace_flags()); } +*/ From ca60fe24d33ca610490b1ba7d41f8f7fdbcad52f Mon Sep 17 00:00:00 2001 From: Karen Xu Date: Tue, 1 Dec 2020 20:22:28 -0500 Subject: [PATCH 13/21] Remove test for default value injection, comments --- sdk/src/logs/logger.cc | 6 +-- sdk/test/logs/BUILD | 1 - sdk/test/logs/logger_provider_sdk_test.cc | 2 +- sdk/test/logs/logger_sdk_test.cc | 51 +++-------------------- 4 files changed, 9 insertions(+), 51 deletions(-) diff --git a/sdk/src/logs/logger.cc b/sdk/src/logs/logger.cc index 953bb1e2a5..e270f8fcba 100644 --- a/sdk/src/logs/logger.cc +++ b/sdk/src/logs/logger.cc @@ -50,10 +50,10 @@ void Logger::Log(opentelemetry::logs::LogRecord &record) noexcept // Inject values into record if not user specified // Timestamp if (r->timestamp == opentelemetry::core::SystemTimestamp(std::chrono::seconds(0))) + { r->timestamp = core::SystemTimestamp(std::chrono::system_clock::now()); + } - - // Inject traceid/spanid (if none is set) auto provider = opentelemetry::trace::Provider::GetTracerProvider(); auto tracer = provider->GetTracer("foo_library"); auto span_context = tracer->GetCurrentSpan()->GetContext(); @@ -76,7 +76,7 @@ void Logger::Log(opentelemetry::logs::LogRecord &record) noexcept r->trace_flags = span_context.trace_flags(); } - // Inject logger name into record + // TODO: Inject logger name into record // Send the log record to the processor processor->OnReceive(r); diff --git a/sdk/test/logs/BUILD b/sdk/test/logs/BUILD index e5cd20ae7c..425d8bcf32 100644 --- a/sdk/test/logs/BUILD +++ b/sdk/test/logs/BUILD @@ -18,7 +18,6 @@ cc_test( deps = [ "//sdk/src/logs", "//sdk/src/trace", - "//exporters/memory:in_memory_span_exporter", "@com_google_googletest//:gtest_main", ], ) diff --git a/sdk/test/logs/logger_provider_sdk_test.cc b/sdk/test/logs/logger_provider_sdk_test.cc index 1a01471234..951ffdb953 100644 --- a/sdk/test/logs/logger_provider_sdk_test.cc +++ b/sdk/test/logs/logger_provider_sdk_test.cc @@ -101,4 +101,4 @@ TEST(LoggerProviderSDK, LoggerLimit) auto logger = lp->GetLogger("Another logger"); opentelemetry::logs::NoopLogger nooplogger; ASSERT_EQ(logger->GetName(), nooplogger.GetName()); -} \ No newline at end of file +} diff --git a/sdk/test/logs/logger_sdk_test.cc b/sdk/test/logs/logger_sdk_test.cc index 831095e3e2..a0712ef33b 100644 --- a/sdk/test/logs/logger_sdk_test.cc +++ b/sdk/test/logs/logger_sdk_test.cc @@ -58,52 +58,11 @@ class DummyProcessor : public LogProcessor void Shutdown(std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept {} }; -/* + TEST(LoggerSDK, DefaultValueInjection) { - // In order to test value injection, the processor must not be nullptr - // A DummyProcessor was created above to satisfy this requirement - std::shared_ptr processor = std::shared_ptr(new DummyProcessor()); - auto lp = std::shared_ptr(new LoggerProvider()); - lp->SetProcessor(processor); - auto logger = lp->GetLogger("Logger1"); - - // Log a sample log record to the processor - opentelemetry::logs::LogRecord r; - r.name = "Test log"; - logger->Log(r); - - // Check that the log record has injected values - // Timestamp shouldn't equal 0 - ASSERT_NE(r.timestamp, opentelemetry::core::SystemTimestamp(std::chrono::seconds(0))); - - //Check that the traceid, spanid, and traceflags are not valid since there is no trace context - ASSERT_FALSE(r.trace_id.IsValid()); - ASSERT_FALSE(r.span_id.IsValid()); - ASSERT_FALSE(r.trace_flags.IsSampled()); - - // To test traceid/spanid/traceflags injection, initialize the tracing pipeline - std::unique_ptr trace_exporter(new opentelemetry::exporter::memory::InMemorySpanExporter()); - auto trace_processor = std::make_shared(std::move(trace_exporter)); - auto trace_provider = opentelemetry::nostd::shared_ptr( - new sdktrace::TracerProvider(trace_processor)); - opentelemetry::trace::Provider::SetTracerProvider(trace_provider); - - //Create a tracer and start a span for span context - auto tracer = trace_provider->GetTracer("foo_library"); - auto span_first = tracer->StartSpan("span 1"); - auto scope_first = tracer->WithActiveSpan(span_first); - auto span_second = tracer->StartSpan("span 2"); - - // Log a sample log record to the processor - opentelemetry::logs::LogRecord r2; - r2.name = "Test log"; - logger->Log(r2); - - //Check that the traceid, spanid, and traceflags were injected from the span context properly - auto span_context = tracer->GetCurrentSpan()->GetContext(); - ASSERT_EQ(r2.trace_id, span_context.trace_id()); - ASSERT_EQ(r2.span_id, span_context.span_id()); - ASSERT_EQ(r2.trace_flags, span_context.trace_flags()); + // TODO: once a Log Exporter is implemented, check that + // timestamp, traceid, spanid, and traceflags were + // injected from the span context properly } -*/ + From a30826aebaac555bc88dc0df39551c8f78d63806 Mon Sep 17 00:00:00 2001 From: Seufert Date: Wed, 2 Dec 2020 09:45:02 -0700 Subject: [PATCH 14/21] Added const to Log(const LogRecord) and const GetName() --- api/include/opentelemetry/logs/logger.h | 4 ++-- api/include/opentelemetry/logs/noop.h | 4 ++-- api/test/logs/logger_test.cc | 4 ++-- sdk/include/opentelemetry/sdk/logs/logger.h | 4 ++-- sdk/src/logs/logger.cc | 4 ++-- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/api/include/opentelemetry/logs/logger.h b/api/include/opentelemetry/logs/logger.h index 0138d0348c..9142142b39 100644 --- a/api/include/opentelemetry/logs/logger.h +++ b/api/include/opentelemetry/logs/logger.h @@ -39,7 +39,7 @@ class Logger virtual ~Logger() = default; /* Returns the name of the logger */ - virtual nostd::string_view GetName() noexcept = 0; + virtual const nostd::string_view GetName() noexcept = 0; /** * Each of the following overloaded log(...) methods @@ -58,7 +58,7 @@ class Logger * A default LogRecord that will be assigned if no parameters are passed to Logger's .log() method * which should at minimum assign the trace_id, span_id, and timestamp */ - virtual void Log(LogRecord &record) noexcept = 0; + virtual void Log(const LogRecord &record) noexcept = 0; /** Overloaded methods for unstructured logging **/ inline void Log(nostd::string_view message) noexcept diff --git a/api/include/opentelemetry/logs/noop.h b/api/include/opentelemetry/logs/noop.h index 4841320d52..43406771d9 100644 --- a/api/include/opentelemetry/logs/noop.h +++ b/api/include/opentelemetry/logs/noop.h @@ -41,8 +41,8 @@ class NoopLogger final : public Logger public: NoopLogger() = default; - void Log(LogRecord& record) noexcept override {} - nostd::string_view GetName() noexcept override { return "noop logger"; }; + void Log(const LogRecord& record) noexcept override {} + const nostd::string_view GetName() noexcept override { return "noop logger"; }; }; /** diff --git a/api/test/logs/logger_test.cc b/api/test/logs/logger_test.cc index 0e3a420df1..4764af1d55 100644 --- a/api/test/logs/logger_test.cc +++ b/api/test/logs/logger_test.cc @@ -83,8 +83,8 @@ TEST(Logger, LogRecordDefaults) // Define a basic Logger class class TestLogger : public Logger { - void Log(LogRecord &record) noexcept override {} - opentelemetry::nostd::string_view GetName() noexcept override { return "test logger"; }; + void Log(const LogRecord &record) noexcept override {} + const opentelemetry::nostd::string_view GetName() noexcept override { return "test logger"; }; }; // Define a basic LoggerProvider class that returns an instance of the logger class defined above diff --git a/sdk/include/opentelemetry/sdk/logs/logger.h b/sdk/include/opentelemetry/sdk/logs/logger.h index b36abd9f79..eb629bd480 100644 --- a/sdk/include/opentelemetry/sdk/logs/logger.h +++ b/sdk/include/opentelemetry/sdk/logs/logger.h @@ -48,13 +48,13 @@ class Logger final : public opentelemetry::logs::Logger /** * Returns the name of this logger. */ - opentelemetry::nostd::string_view GetName() noexcept override; + const opentelemetry::nostd::string_view GetName() noexcept override; /** * Writes a log record into the processor. * @param record The record to write into the processor. */ - void Log(opentelemetry::logs::LogRecord &record) noexcept override; + void Log(const opentelemetry::logs::LogRecord &record) noexcept override; private: // The logger provider of this Logger. Uses a weak_ptr to avoid cyclic dependancy issues the with diff --git a/sdk/src/logs/logger.cc b/sdk/src/logs/logger.cc index 953bb1e2a5..e94fc78646 100644 --- a/sdk/src/logs/logger.cc +++ b/sdk/src/logs/logger.cc @@ -28,12 +28,12 @@ Logger::Logger(opentelemetry::nostd::string_view name, : logger_name_(name), logger_provider_(logger_provider) {} -opentelemetry::nostd::string_view Logger::GetName() noexcept +const opentelemetry::nostd::string_view Logger::GetName() noexcept { return logger_name_; } -void Logger::Log(opentelemetry::logs::LogRecord &record) noexcept +void Logger::Log(const opentelemetry::logs::LogRecord &record) noexcept { // If this logger does not have a processor, no need to create a log record auto processor = logger_provider_.lock()->GetProcessor(); From 36f7ba886ef470e7c2446f6e4545eeb61f685be1 Mon Sep 17 00:00:00 2001 From: Karen Xu Date: Wed, 2 Dec 2020 14:50:15 -0500 Subject: [PATCH 15/21] Change test names for better conventions / consistency --- api/test/logs/BUILD | 4 +- api/test/logs/CMakeLists.txt | 8 +-- api/test/logs/logger_test.cc | 60 +++++++++---------- ...gger_provider_test.cc => provider_test.cc} | 0 sdk/test/logs/BUILD | 8 +-- sdk/test/logs/CMakeLists.txt | 4 +- ...er_sdk_test.cc => logger_provider_test.cc} | 10 ++-- .../{logger_sdk_test.cc => logger_test.cc} | 6 +- 8 files changed, 50 insertions(+), 50 deletions(-) rename api/test/logs/{logger_provider_test.cc => provider_test.cc} (100%) rename sdk/test/logs/{logger_provider_sdk_test.cc => logger_provider_test.cc} (93%) rename sdk/test/logs/{logger_sdk_test.cc => logger_test.cc} (95%) diff --git a/api/test/logs/BUILD b/api/test/logs/BUILD index 899400b2e8..cba6f19220 100644 --- a/api/test/logs/BUILD +++ b/api/test/logs/BUILD @@ -1,9 +1,9 @@ load("//bazel:otel_cc_benchmark.bzl", "otel_cc_benchmark") cc_test( - name = "logger_provider_test", + name = "provider_test", srcs = [ - "logger_provider_test.cc", + "provider_test.cc", ], deps = [ "//api", diff --git a/api/test/logs/CMakeLists.txt b/api/test/logs/CMakeLists.txt index 737edb893c..8f797b97c1 100644 --- a/api/test/logs/CMakeLists.txt +++ b/api/test/logs/CMakeLists.txt @@ -1,6 +1,6 @@ -foreach(testname logger_provider_test logger_test) - add_executable(${testname} "${testname}.cc") - target_link_libraries(${testname} ${GTEST_BOTH_LIBRARIES} +foreach(testname provider_test logger_test) + add_executable(logs_api_${testname} "${testname}.cc") + target_link_libraries(logs_api_${testname} ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT} opentelemetry_api) - gtest_add_tests(TARGET ${testname} TEST_PREFIX logs. TEST_LIST ${testname}) + gtest_add_tests(TARGET logs_api_${testname} TEST_PREFIX logs. TEST_LIST logs_api_${testname}) endforeach() diff --git a/api/test/logs/logger_test.cc b/api/test/logs/logger_test.cc index 0e3a420df1..b3ea4b2d1b 100644 --- a/api/test/logs/logger_test.cc +++ b/api/test/logs/logger_test.cc @@ -15,7 +15,7 @@ using opentelemetry::nostd::shared_ptr; using opentelemetry::nostd::span; using opentelemetry::nostd::string_view; -TEST(Logger, GetLoggerDefaultNoop) +TEST(LoggerTest, GetLoggerDefaultNoop) { auto lp = Provider::GetLoggerProvider(); auto logger = lp->GetLogger("TestLogger"); @@ -23,7 +23,7 @@ TEST(Logger, GetLoggerDefaultNoop) EXPECT_EQ(logger->GetName(), "noop logger"); } -TEST(Logger, GetLoggerMethod) +TEST(LoggerTest, GetLogger) { auto lp = Provider::GetLoggerProvider(); @@ -39,7 +39,7 @@ TEST(Logger, GetLoggerMethod) auto logger3 = lp->GetLogger("TestLogger3", args); } -TEST(Logger, LogMethod) +TEST(LoggerTest, Log) { auto lp = Provider::GetLoggerProvider(); auto logger = lp->GetLogger("TestLogger"); @@ -54,32 +54,6 @@ TEST(Logger, LogMethod) logger->Log(r); } -TEST(Logger, LogRecordDefaults) -{ - LogRecord r; - - // Check that the timestamp is set to 0 by default - ASSERT_EQ(r.timestamp, opentelemetry::core::SystemTimestamp(std::chrono::seconds(0))); - - // Check that the severity is set to kDefault by default - ASSERT_EQ(r.severity, Severity::kDefault); - - // Check that trace_id is set to all zeros by default - char trace_buf[32]; - r.trace_id.ToLowerBase16(trace_buf); - ASSERT_EQ(std::string(trace_buf, sizeof(trace_buf)), "00000000000000000000000000000000"); - - // Check that span_id is set to all zeros by default - char span_buf[16]; - r.span_id.ToLowerBase16(span_buf); - ASSERT_EQ(std::string(span_buf, sizeof(span_buf)), "0000000000000000"); - - // Check that trace_flags is set to all zeros by default - char flags_buf[2]; - r.trace_flags.ToLowerBase16(flags_buf); - ASSERT_EQ(std::string(flags_buf, sizeof(flags_buf)), "00"); -} - // Define a basic Logger class class TestLogger : public Logger { @@ -101,7 +75,7 @@ class TestProvider : public LoggerProvider } }; -TEST(Logger, PushLoggerImplementation) +TEST(LoggerTest, PushLoggerImplementation) { // Push the new loggerprovider class into the API auto test_provider = shared_ptr(new TestProvider()); @@ -114,3 +88,29 @@ TEST(Logger, PushLoggerImplementation) auto logger = lp->GetLogger("TestLogger"); ASSERT_EQ("test logger", logger->GetName()); } + +TEST(LogRecord, SetDefault) +{ + LogRecord r; + + // Check that the timestamp is set to 0 by default + ASSERT_EQ(r.timestamp, opentelemetry::core::SystemTimestamp(std::chrono::seconds(0))); + + // Check that the severity is set to kDefault by default + ASSERT_EQ(r.severity, Severity::kDefault); + + // Check that trace_id is set to all zeros by default + char trace_buf[32]; + r.trace_id.ToLowerBase16(trace_buf); + ASSERT_EQ(std::string(trace_buf, sizeof(trace_buf)), "00000000000000000000000000000000"); + + // Check that span_id is set to all zeros by default + char span_buf[16]; + r.span_id.ToLowerBase16(span_buf); + ASSERT_EQ(std::string(span_buf, sizeof(span_buf)), "0000000000000000"); + + // Check that trace_flags is set to all zeros by default + char flags_buf[2]; + r.trace_flags.ToLowerBase16(flags_buf); + ASSERT_EQ(std::string(flags_buf, sizeof(flags_buf)), "00"); +} diff --git a/api/test/logs/logger_provider_test.cc b/api/test/logs/provider_test.cc similarity index 100% rename from api/test/logs/logger_provider_test.cc rename to api/test/logs/provider_test.cc diff --git a/sdk/test/logs/BUILD b/sdk/test/logs/BUILD index 425d8bcf32..ca09b3d070 100644 --- a/sdk/test/logs/BUILD +++ b/sdk/test/logs/BUILD @@ -1,7 +1,7 @@ cc_test( - name = "logger_provider_sdk_test", + name = "logger_provider_test", srcs = [ - "logger_provider_sdk_test.cc", + "logger_provider_test.cc", ], deps = [ "//api", @@ -11,9 +11,9 @@ cc_test( ) cc_test( - name = "logger_sdk_test", + name = "logger_test", srcs = [ - "logger_sdk_test.cc", + "logger_test.cc", ], deps = [ "//sdk/src/logs", diff --git a/sdk/test/logs/CMakeLists.txt b/sdk/test/logs/CMakeLists.txt index 5de4ae2ee1..1c90b76e69 100644 --- a/sdk/test/logs/CMakeLists.txt +++ b/sdk/test/logs/CMakeLists.txt @@ -1,7 +1,7 @@ foreach( testname - logger_provider_sdk_test - logger_sdk_test) + logger_provider_test + logger_test) add_executable(${testname} "${testname}.cc") target_link_libraries(${testname} ${GTEST_BOTH_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT} opentelemetry_logs opentelemetry_exporter_in_memory) diff --git a/sdk/test/logs/logger_provider_sdk_test.cc b/sdk/test/logs/logger_provider_test.cc similarity index 93% rename from sdk/test/logs/logger_provider_sdk_test.cc rename to sdk/test/logs/logger_provider_test.cc index 951ffdb953..fbe1918977 100644 --- a/sdk/test/logs/logger_provider_sdk_test.cc +++ b/sdk/test/logs/logger_provider_test.cc @@ -23,7 +23,7 @@ using namespace opentelemetry::sdk::logs; -TEST(LoggerProviderSDK, PushToAPI) +TEST(LoggerProvider, PushToAPI) { auto lp = opentelemetry::nostd::shared_ptr( new opentelemetry::sdk::logs::LoggerProvider()); @@ -33,7 +33,7 @@ TEST(LoggerProviderSDK, PushToAPI) ASSERT_EQ(lp, opentelemetry::logs::Provider::GetLoggerProvider()); } -TEST(LoggerProviderSDK, LoggerProviderGetLoggerSimple) +TEST(LoggerProvider, LoggerProviderGetLoggerSimple) { auto lp = std::shared_ptr(new LoggerProvider()); @@ -52,7 +52,7 @@ TEST(LoggerProviderSDK, LoggerProviderGetLoggerSimple) ASSERT_EQ(logger1, logger3); } -TEST(LoggerProviderSDK, LoggerProviderLoggerArguments) +TEST(LoggerProvider, LoggerProviderLoggerArguments) { // Currently, arguments are not supported by the loggers. // TODO: Once the logging spec defines what arguments are allowed, add more @@ -75,7 +75,7 @@ class DummyProcessor : public LogProcessor void Shutdown(std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept {} }; -TEST(LoggerProviderSDK, GetAndSetProcessor) +TEST(LoggerProvider, GetAndSetProcessor) { // Create a LoggerProvider without a processor LoggerProvider lp; @@ -87,7 +87,7 @@ TEST(LoggerProviderSDK, GetAndSetProcessor) ASSERT_EQ(proc2, lp.GetProcessor()); } -TEST(LoggerProviderSDK, LoggerLimit) +TEST(LoggerProvider, LoggerLimit) { auto lp = std::shared_ptr(new LoggerProvider()); diff --git a/sdk/test/logs/logger_sdk_test.cc b/sdk/test/logs/logger_test.cc similarity index 95% rename from sdk/test/logs/logger_sdk_test.cc rename to sdk/test/logs/logger_test.cc index a0712ef33b..6324eb4e95 100644 --- a/sdk/test/logs/logger_sdk_test.cc +++ b/sdk/test/logs/logger_test.cc @@ -25,7 +25,7 @@ using namespace opentelemetry::sdk::logs; namespace sdktrace = opentelemetry::sdk::trace; -TEST(LoggerSDK, LoggerName) +TEST(Logger, LoggerName) { auto lp = std::shared_ptr(new LoggerProvider()); auto logger1 = lp->GetLogger("logger1"); @@ -35,7 +35,7 @@ TEST(LoggerSDK, LoggerName) ASSERT_EQ("logger2", logger2->GetName()); } -TEST(LoggerSDK, LogToNullProcessor) +TEST(Logger, LogToNullProcessor) { // Confirm Logger::log() does not have undefined behavior // even when there is no processor set @@ -59,7 +59,7 @@ class DummyProcessor : public LogProcessor }; -TEST(LoggerSDK, DefaultValueInjection) +TEST(Logger, DefaultValueInjection) { // TODO: once a Log Exporter is implemented, check that // timestamp, traceid, spanid, and traceflags were From d3d13b4fb3e86826ee541fb4478f672abbb30d96 Mon Sep 17 00:00:00 2001 From: Karen Xu Date: Wed, 2 Dec 2020 16:45:50 -0500 Subject: [PATCH 16/21] change processor signatures for tests --- sdk/test/logs/logger_provider_test.cc | 4 ++-- sdk/test/logs/logger_test.cc | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/sdk/test/logs/logger_provider_test.cc b/sdk/test/logs/logger_provider_test.cc index fbe1918977..1e1c44f02d 100644 --- a/sdk/test/logs/logger_provider_test.cc +++ b/sdk/test/logs/logger_provider_test.cc @@ -71,8 +71,8 @@ class DummyProcessor : public LogProcessor { void OnReceive(opentelemetry::nostd::shared_ptr record) noexcept {} - void ForceFlush(std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept {} - void Shutdown(std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept {} + bool ForceFlush(std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept {} + bool Shutdown(std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept {} }; TEST(LoggerProvider, GetAndSetProcessor) diff --git a/sdk/test/logs/logger_test.cc b/sdk/test/logs/logger_test.cc index 6324eb4e95..2a724c3c78 100644 --- a/sdk/test/logs/logger_test.cc +++ b/sdk/test/logs/logger_test.cc @@ -54,8 +54,8 @@ class DummyProcessor : public LogProcessor { void OnReceive(opentelemetry::nostd::shared_ptr record) noexcept {} - void ForceFlush(std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept {} - void Shutdown(std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept {} + bool ForceFlush(std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept {} + bool Shutdown(std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept {} }; From 0c97eae9f0fd0e5e150e8f2158b0305d74ba4b30 Mon Sep 17 00:00:00 2001 From: Karen Xu Date: Wed, 2 Dec 2020 16:47:55 -0500 Subject: [PATCH 17/21] Remove inMem exporter from build files --- sdk/test/logs/CMakeLists.txt | 2 +- sdk/test/logs/logger_test.cc | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/sdk/test/logs/CMakeLists.txt b/sdk/test/logs/CMakeLists.txt index 1c90b76e69..4bda950b00 100644 --- a/sdk/test/logs/CMakeLists.txt +++ b/sdk/test/logs/CMakeLists.txt @@ -4,6 +4,6 @@ foreach( logger_test) add_executable(${testname} "${testname}.cc") target_link_libraries(${testname} ${GTEST_BOTH_LIBRARIES} - ${CMAKE_THREAD_LIBS_INIT} opentelemetry_logs opentelemetry_exporter_in_memory) + ${CMAKE_THREAD_LIBS_INIT} opentelemetry_logs) gtest_add_tests(TARGET ${testname} TEST_PREFIX logs. TEST_LIST ${testname}) endforeach() diff --git a/sdk/test/logs/logger_test.cc b/sdk/test/logs/logger_test.cc index 2a724c3c78..bc65050c19 100644 --- a/sdk/test/logs/logger_test.cc +++ b/sdk/test/logs/logger_test.cc @@ -17,7 +17,6 @@ #include "opentelemetry/sdk/logs/logger.h" #include "opentelemetry/sdk/trace/tracer_provider.h" #include "opentelemetry/trace/provider.h" -#include "opentelemetry/exporters/memory/in_memory_span_exporter.h" #include "opentelemetry/sdk/trace/simple_processor.h" #include From e153de5d6b2894143849f8775c25dadaaa114451 Mon Sep 17 00:00:00 2001 From: Seufert Date: Wed, 2 Dec 2020 14:57:58 -0700 Subject: [PATCH 18/21] Made processor.h interface match the processor PR --- .../opentelemetry/sdk/logs/processor.h | 32 ++++++++++++++----- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/sdk/include/opentelemetry/sdk/logs/processor.h b/sdk/include/opentelemetry/sdk/logs/processor.h index 47b37baed0..42cc3008c8 100644 --- a/sdk/include/opentelemetry/sdk/logs/processor.h +++ b/sdk/include/opentelemetry/sdk/logs/processor.h @@ -26,22 +26,38 @@ namespace sdk namespace logs { /** - * This Log Processor is responsible for conversion of logs to exportable - * representation and passing them to exporters. + * The Log Processor is responsible for passing log records + * to the configured exporter. */ class LogProcessor { public: virtual ~LogProcessor() = default; - virtual void OnReceive(nostd::shared_ptr record) noexcept = 0; + /** + * OnReceive is called by the SDK once a log record has been successfully created. + * @param record the log record + */ + virtual void OnReceive(std::unique_ptr &&record) noexcept = 0; - virtual void ForceFlush( - std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept = 0; + /** + * Exports all log records that have not yet been exported to the configured Exporter. + * @param timeout that the forceflush is required to finish within. + * @return a result code indicating whether it succeeded, failed or timed out + */ + virtual bool ForceFlush( + std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept = 0; - virtual void Shutdown( - std::chrono::microseconds timeout = std::chrono::microseconds(0)) noexcept = 0; + /** + * Shuts down the processor and does any cleanup required. + * ShutDown should only be called once for each processor. + * @param timeout minimum amount of microseconds to wait for + * shutdown before giving up and returning failure. + * @return true if the shutdown succeeded, false otherwise + */ + virtual bool Shutdown( + std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept = 0; }; } // namespace logs } // namespace sdk -OPENTELEMETRY_END_NAMESPACE +OPENTELEMETRY_END_NAMESPACE \ No newline at end of file From d2cbd0f5c39f64c52c0bdf0914802a4344b496e9 Mon Sep 17 00:00:00 2001 From: Seufert Date: Wed, 2 Dec 2020 15:26:39 -0700 Subject: [PATCH 19/21] Changed processor back to using unique_ptr from shared_ptr --- sdk/src/logs/logger.cc | 21 +++++++++++---------- sdk/test/logs/logger_provider_test.cc | 2 +- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/sdk/src/logs/logger.cc b/sdk/src/logs/logger.cc index d6b32c916c..c88a73de35 100644 --- a/sdk/src/logs/logger.cc +++ b/sdk/src/logs/logger.cc @@ -45,13 +45,14 @@ void Logger::Log(const opentelemetry::logs::LogRecord &record) noexcept // TODO: Sampler logic (should include check for minSeverity) // Create a shared pointer to the LogRecord to be passed to the processor(s) - auto r = opentelemetry::nostd::shared_ptr(new opentelemetry::logs::LogRecord(record)); + auto record_pointer = + std::unique_ptr(new opentelemetry::logs::LogRecord(record)); // Inject values into record if not user specified // Timestamp - if (r->timestamp == opentelemetry::core::SystemTimestamp(std::chrono::seconds(0))) + if (record_pointer->timestamp == opentelemetry::core::SystemTimestamp(std::chrono::seconds(0))) { - r->timestamp = core::SystemTimestamp(std::chrono::system_clock::now()); + record_pointer->timestamp = core::SystemTimestamp(std::chrono::system_clock::now()); } auto provider = opentelemetry::trace::Provider::GetTracerProvider(); @@ -59,27 +60,27 @@ void Logger::Log(const opentelemetry::logs::LogRecord &record) noexcept auto span_context = tracer->GetCurrentSpan()->GetContext(); // Traceid - if (!r->trace_id.IsValid()) + if (!record_pointer->trace_id.IsValid()) { - r->trace_id = span_context.trace_id(); + record_pointer->trace_id = span_context.trace_id(); } // Spanid - if (!r->span_id.IsValid()) + if (!record_pointer->span_id.IsValid()) { - r->span_id = span_context.span_id(); + record_pointer->span_id = span_context.span_id(); } // Traceflag - if (!r->trace_flags.IsSampled()) + if (!record_pointer->trace_flags.IsSampled()) { - r->trace_flags = span_context.trace_flags(); + record_pointer->trace_flags = span_context.trace_flags(); } // TODO: Inject logger name into record // Send the log record to the processor - processor->OnReceive(r); + processor->OnReceive(std::move(record_pointer)); } } // namespace logs diff --git a/sdk/test/logs/logger_provider_test.cc b/sdk/test/logs/logger_provider_test.cc index 1e1c44f02d..29b4e87e06 100644 --- a/sdk/test/logs/logger_provider_test.cc +++ b/sdk/test/logs/logger_provider_test.cc @@ -69,7 +69,7 @@ TEST(LoggerProvider, LoggerProviderLoggerArguments) class DummyProcessor : public LogProcessor { - void OnReceive(opentelemetry::nostd::shared_ptr record) noexcept + void OnReceive(std::unique_ptr &&record) noexcept {} bool ForceFlush(std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept {} bool Shutdown(std::chrono::microseconds timeout = std::chrono::microseconds::max()) noexcept {} From 206e8ca2d0164b83d759cc6d0e15f6d7209d555a Mon Sep 17 00:00:00 2001 From: Seufert Date: Wed, 2 Dec 2020 18:39:26 -0700 Subject: [PATCH 20/21] Modified Log overloads, fixed LogRecord KViterable --- api/include/opentelemetry/logs/log_record.h | 11 +-- api/include/opentelemetry/logs/logger.h | 83 +++++++++++++-------- api/test/logs/logger_test.cc | 46 ++++++++---- 3 files changed, 85 insertions(+), 55 deletions(-) diff --git a/api/include/opentelemetry/logs/log_record.h b/api/include/opentelemetry/logs/log_record.h index f150848344..f8d905ba6e 100644 --- a/api/include/opentelemetry/logs/log_record.h +++ b/api/include/opentelemetry/logs/log_record.h @@ -65,11 +65,6 @@ enum class Severity : uint8_t kDefault = kInfo // default severity is set to kInfo level, similar to what is done in ILogger }; -/* _nullKV is defined as a private variable that allows "resource" and - "attributes" fields to be instantiated using it as the default value */ -static common::KeyValueIterableView> _nullKV = - common::KeyValueIterableView>{{}}; - /** * A default Event object to be passed in log statements, * matching the 10 fields of the Log Data Model. @@ -88,15 +83,15 @@ struct LogRecord // other fields that will not be set by default nostd::string_view name; // string nostd::string_view body; // currently a simple string, but should be changed "Any" type - common::KeyValueIterable &resource; // key/value pair list - common::KeyValueIterable &attributes; // key/value pair list + nostd::shared_ptr resource; // key/value pair list + nostd::shared_ptr attributes; // key/value pair list /* Default log record if user does not overwrite this. * TODO: find better data type to represent the type for "body" * Future enhancement: Potentially add other constructors to take default arguments * from the user **/ - LogRecord() : resource(_nullKV), attributes(_nullKV) + LogRecord() { // Assign default values timestamp = core::SystemTimestamp(std::chrono::seconds(0)); diff --git a/api/include/opentelemetry/logs/logger.h b/api/include/opentelemetry/logs/logger.h index 9142142b39..49af29c3bd 100644 --- a/api/include/opentelemetry/logs/logger.h +++ b/api/include/opentelemetry/logs/logger.h @@ -54,64 +54,83 @@ class Logger * @throws No exceptions under any circumstances. */ - /* The below method is a logging statement that takes in a LogRecord. - * A default LogRecord that will be assigned if no parameters are passed to Logger's .log() method - * which should at minimum assign the trace_id, span_id, and timestamp + /** + * Logs a LogRecord, which contains all the fields of the Log Data Model. Normally called indirectly from other Log() Methods, but can be called directly for high detail. + * @param record A log record filled with information from the user. */ virtual void Log(const LogRecord &record) noexcept = 0; - /** Overloaded methods for unstructured logging **/ - inline void Log(nostd::string_view message) noexcept - { - // Set severity to the default then call log(Severity, String message) method - Log(Severity::kDefault, message); - } + ///Overloaded Log methods, which are simpler than calling a LogRecord directly + /** + * Writes a log. + * @param severity The log's severity + * @param message The message to log + */ inline void Log(Severity severity, nostd::string_view message) noexcept { - // creates a LogRecord object with given parameters, then calls Log(LogRecord) + //Create a LogRecord to hold this information LogRecord r; - r.severity = severity; - r.body = message; + r.severity = severity; + r.body = message; + //Call the main Log(LogRecord) method Log(r); } - inline void Log(Severity severity, - nostd::string_view message, - core::SystemTimestamp time) noexcept + /** + * Writes a log. + * @param severity The log's severity + * @param name The name of the log + * @param message The message to log + */ + inline void Log(Severity severity, nostd::string_view name, nostd::string_view message) noexcept { - // creates a LogRecord object with given parameters, then calls Log(LogRecord) + //Create a LogRecord to hold this information LogRecord r; - r.severity = severity; - r.body = message; - r.timestamp = time; + r.severity = severity; + r.name = name; + r.body = message; + //Call the main Log(LogRecord) method Log(r); } - /** Overloaded methods for structured logging**/ - // TODO: separate this method into separate methods since it is not useful for user to create - // empty logs + /** + * Writes a log. + * @param severity The severity of the log, from 1 to 24 + * @param attributes A key/value object + */ template ::value> * = nullptr> - inline void Log(Severity severity = Severity::kDefault, - nostd::string_view name = "", - const T &attributes = {}) noexcept + nostd::enable_if_t::value> * = nullptr> + inline void Log(Severity severity, const T &attributes) noexcept { - Log(severity, name, common::KeyValueIterableView(attributes)); + //Create a LogRecord to hold this information + LogRecord r; + r.severity = severity; + r.attributes = nostd::shared_ptr(new common::KeyValueIterableView{attributes}); + + //Call the main Log(LogRecord) method + Log(r); } - inline void Log(Severity severity, - nostd::string_view name, - const common::KeyValueIterable &attributes) noexcept + /** + * Writes a log. + * @param severity The severity of the log, from 1 to 24 + * @param name The name of the log + * @param attributes A key/value object + */ + template ::value> * = nullptr> + inline void Log(Severity severity, nostd::string_view name, const T &attributes) noexcept { - // creates a LogRecord object with given parameters, then calls log(LogRecord) + //Create a LogRecord to hold this information LogRecord r; r.severity = severity; r.name = name; - r.attributes = attributes; + r.attributes = nostd::shared_ptr(new common::KeyValueIterableView{attributes}); + //Call the main Log(LogRecord) method Log(r); } diff --git a/api/test/logs/logger_test.cc b/api/test/logs/logger_test.cc index 7eb0937a11..cf7a572b4e 100644 --- a/api/test/logs/logger_test.cc +++ b/api/test/logs/logger_test.cc @@ -39,25 +39,16 @@ TEST(LoggerTest, GetLogger) auto logger3 = lp->GetLogger("TestLogger3", args); } -TEST(LoggerTest, Log) -{ - auto lp = Provider::GetLoggerProvider(); - auto logger = lp->GetLogger("TestLogger"); - - // Test log(severity, name) method - logger->Log(Severity::kError, "Error message"); - - // Test log(LogRecord) - LogRecord r; - r.name = "Log Record"; - r.severity = Severity::kInfo; - logger->Log(r); -} +// Define a global log record that will be modified when the Log() method is called +static opentelemetry::nostd::shared_ptr record_; // Define a basic Logger class class TestLogger : public Logger { - void Log(const LogRecord &record) noexcept override {} + void Log(const LogRecord &record) noexcept override + { + record_ = opentelemetry::nostd::shared_ptr(new LogRecord(record)); + } const opentelemetry::nostd::string_view GetName() noexcept override { return "test logger"; }; }; @@ -89,6 +80,31 @@ TEST(LoggerTest, PushLoggerImplementation) ASSERT_EQ("test logger", logger->GetName()); } +TEST(Logger, LogMethodOverloads) +{ + // Use the same TestProvider and TestLogger from the previous test + auto test_provider = shared_ptr(new TestProvider()); + Provider::SetLoggerProvider(test_provider); + + auto lp = Provider::GetLoggerProvider(); + auto logger = lp->GetLogger("TestLogger"); + + // Check that calling the Log() overloads correctly constructs a log record which is automatically put into the static logger_ for testing + + // Test Log(severity, name, message) method + logger->Log(Severity::kError, "Log Name", "This is the log message"); + ASSERT_EQ(record_->severity, Severity::kError); + ASSERT_EQ(record_->name, "Log Name"); + ASSERT_EQ(record_->body, "This is the log message"); + + // Test Log(severity, name, KVIterable) method + std::map m1 = {{"key1", "val1"}, {"key2", "val2"}}; + logger->Log(Severity::kWarn, "Logging a map", m1); + ASSERT_EQ(record_->severity, Severity::kWarn); + ASSERT_EQ(record_->name, "Logging a map"); + ASSERT_EQ(record_->attributes->size(), 2); +} + TEST(LogRecord, SetDefault) { LogRecord r; From 30affaace82aa7ca1459e7ea92fe2c057931f01f Mon Sep 17 00:00:00 2001 From: Seufert Date: Wed, 2 Dec 2020 18:54:23 -0700 Subject: [PATCH 21/21] Added logger.Trace(~) overloads and test --- api/include/opentelemetry/logs/logger.h | 47 +++++++++++++++++++++++-- api/test/logs/logger_test.cc | 8 ++--- 2 files changed, 48 insertions(+), 7 deletions(-) diff --git a/api/include/opentelemetry/logs/logger.h b/api/include/opentelemetry/logs/logger.h index 49af29c3bd..ec592c7a54 100644 --- a/api/include/opentelemetry/logs/logger.h +++ b/api/include/opentelemetry/logs/logger.h @@ -61,7 +61,7 @@ class Logger virtual void Log(const LogRecord &record) noexcept = 0; ///Overloaded Log methods, which are simpler than calling a LogRecord directly - + /** * Writes a log. * @param severity The log's severity @@ -134,8 +134,49 @@ class Logger Log(r); } - // TODO: add function aliases such as void debug(), void trace(), void info(), etc. for each - // severity level + ///Trace overloads + + /** + * Writes a log with a severity of trace. + * @param message The message to log + */ + inline void Trace(nostd::string_view message) noexcept + { + Log(Severity::kTrace, message); + } + + /** + * Writes a log with a severity of trace. + * @param name The name of the log + * @param message The message to log + */ + inline void Trace(nostd::string_view name, nostd::string_view message) noexcept + { + Log(Severity::kTrace, name, message); + } + + /** + * Writes a log with a severity of trace. + * @param attributes A key/value object + */ + template ::value> * = nullptr> + inline void Trace(const T &attributes) noexcept + { + Log(Severity::kTrace, attributes); + } + + /** + * Writes a log with a severity of trace. + * @param name The name of the log + * @param attributes A key/value object + */ + template ::value> * = nullptr> + inline void Trace(nostd::string_view name, const T &attributes) noexcept + { + Log(Severity::kTrace, name, attributes); + } /** Future enhancement: templated method for objects / custom types (e.g. JSON, XML, custom * classes, etc) **/ diff --git a/api/test/logs/logger_test.cc b/api/test/logs/logger_test.cc index cf7a572b4e..b05e333a93 100644 --- a/api/test/logs/logger_test.cc +++ b/api/test/logs/logger_test.cc @@ -97,10 +97,10 @@ TEST(Logger, LogMethodOverloads) ASSERT_EQ(record_->name, "Log Name"); ASSERT_EQ(record_->body, "This is the log message"); - // Test Log(severity, name, KVIterable) method - std::map m1 = {{"key1", "val1"}, {"key2", "val2"}}; - logger->Log(Severity::kWarn, "Logging a map", m1); - ASSERT_EQ(record_->severity, Severity::kWarn); + // Test Trace(name, KVIterable) method + std::map m = {{"key1", "val1"}, {"key2", "val2"}}; + logger->Trace("Logging a map", m); + ASSERT_EQ(record_->severity, Severity::kTrace); ASSERT_EQ(record_->name, "Logging a map"); ASSERT_EQ(record_->attributes->size(), 2); }