Logging API and SDK modifications

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<std::map<nostd::string_view, nostd::string_view>> _nullKV =
-    common::KeyValueIterableView<std::map<nostd::string_view, nostd::string_view>>{{}};
-
 /**
  * A default Event object to be passed in log statements,
  * matching the 10 fields of the Log Data Model.
@@ -82,25 +77,28 @@ 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
   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<common::KeyValueIterable> resource;    // key/value pair list
+  nostd::shared_ptr<common::KeyValueIterable> attributes;  // key/value pair list
 
   /* Default log record if user does not overwrite this.
    * TODO: find better data type to represent the <Any> type for "body"
    * Future enhancement: Potentially add other constructors to take default arguments
    * from the user
    **/
-  LogRecord() : resource(_nullKV), attributes(_nullKV)
+  LogRecord()
   {
-    // TODO: in SDK, assign a default timestamp if not specified
-    name = "";
+    // 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

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 const nostd::string_view GetName() noexcept = 0;
 
   /**
    * Each of the following overloaded log(...) methods
@@ -55,65 +54,129 @@ 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
+  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
+   * @param message The message to log
+   */
+  inline void Log(Severity severity, nostd::string_view message) noexcept
   {
-    // Set severity to the default then call log(Severity, String message) method
-    log(Severity::kDefault, message);
-  }
+    //Create a LogRecord to hold this information
+    LogRecord r;
+    r.severity   = severity;
+    r.body    = message;
 
-  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()));
+    //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;
 
-    log(r);
+    //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 <class T,
-            nostd::enable_if_t<common::detail::is_key_value_iterable<T>::value> * = nullptr>
-  inline void log(Severity severity       = Severity::kDefault,
-                  nostd::string_view name = "",
-                  const T &attributes     = {}) noexcept
+          nostd::enable_if_t<common::detail::is_key_value_iterable<T>::value> * = nullptr>
+  inline void Log(Severity severity, const T &attributes) noexcept
   {
-    log(severity, name, common::KeyValueIterableView<T>(attributes));
+    //Create a LogRecord to hold this information
+    LogRecord r;
+    r.severity = severity;
+    r.attributes = nostd::shared_ptr<common::KeyValueIterable>(new common::KeyValueIterableView<T>{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 <class T,
+          nostd::enable_if_t<common::detail::is_key_value_iterable<T>::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<common::KeyValueIterable>(new common::KeyValueIterableView<T>{attributes});
+
+    //Call the main Log(LogRecord) method
+    Log(r);
+  }
+
+  ///Trace overloads
 
-    log(r);
+  /** 
+   * 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);
   }
 
-  // TODO: add function aliases such as void debug(), void trace(), void info(), etc. for each
-  // severity level
+  /** 
+   * Writes a log with a severity of trace.
+   * @param attributes A key/value object
+   */
+  template <class T,
+          nostd::enable_if_t<common::detail::is_key_value_iterable<T>::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 <class T,
+          nostd::enable_if_t<common::detail::is_key_value_iterable<T>::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) **/

api/include/opentelemetry/logs/noop.h

@@ -41,7 +41,8 @@ class NoopLogger final : public Logger
 public:
   NoopLogger() = default;
 
-  void log(const LogRecord &record) noexcept override {}
+  void Log(const LogRecord& record) noexcept override {}
+  const nostd::string_view GetName() noexcept override { return "noop logger"; };
 };
 
 /**

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",

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()

api/test/logs/logger_test.cc

@@ -15,43 +15,41 @@ using opentelemetry::nostd::shared_ptr;
 using opentelemetry::nostd::span;
 using opentelemetry::nostd::string_view;
 
-TEST(Logger, GetLoggerDefault)
+TEST(LoggerTest, GetLoggerDefaultNoop)
 {
   auto lp     = Provider::GetLoggerProvider();
   auto logger = lp->GetLogger("TestLogger");
   EXPECT_NE(nullptr, logger);
+  EXPECT_EQ(logger->GetName(), "noop logger");
 }
 
-TEST(Logger, GetNoopLoggerName)
-{
-  auto lp     = Provider::GetLoggerProvider();
-  auto logger = lp->GetLogger("TestLogger");
-}
-
-TEST(Logger, GetNoopLoggerNameWithArgs)
+TEST(LoggerTest, GetLogger)
 {
   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<string_view, 1> sv{"string"};
   span<string_view> 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)
-{
-  auto lp     = Provider::GetLoggerProvider();
-  auto logger = lp->GetLogger("TestLogger");
-  LogRecord r;
-  r.name = "Noop log name";
-  logger->log(r);
-}
+// Define a global log record that will be modified when the Log() method is called
+static opentelemetry::nostd::shared_ptr<LogRecord> 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<LogRecord>(new LogRecord(record));
+  }
+  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
@@ -68,19 +66,67 @@ class TestProvider : public LoggerProvider
   }
 };
 
-TEST(Logger, PushLoggerImplementation)
+TEST(LoggerTest, PushLoggerImplementation)
 {
-  // Push the new loggerprovider class into the global singleton
+  // Push the new loggerprovider class into the API
   auto test_provider = shared_ptr<LoggerProvider>(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");
+  ASSERT_EQ("test logger", logger->GetName());
+}
 
-  // GetLogger(name, args) function
-  std::array<string_view, 1> sv{"string"};
-  span<string_view> args{sv};
-  auto logger2 = lp->GetLogger("TestLogger2", args);
+TEST(Logger, LogMethodOverloads)
+{
+  // Use the same TestProvider and TestLogger from the previous test
+  auto test_provider = shared_ptr<LoggerProvider>(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 Trace(name, KVIterable) method
+  std::map<std::string, std::string> 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);
+}
+
+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");
 }