Merge branch 'main' into fix-install-sdk-config

open-telemetry · Jun 6, 2022 · eca54eb · eca54eb
2 parents 15e3d9f + 7e90dae
commit eca54eb
Show file tree

Hide file tree

Showing 17 changed files with 135 additions and 39 deletions.
diff --git a/.gitattributes b/.gitattributes
@@ -41,3 +41,7 @@ LICENSE*                text
 ## git files
 .gitignore              text eol=lf
 .gitattributes          text eol=lf
+
+## bazel files
+WORKSPACE               text eol=lf
+BUILD                   text eol=lf
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,8 @@ Increment the:
 
 ## [Unreleased]
 
+* [METRICS] Only record non-negative / finite / Non-NAN histogram values([#1427](https://github.com/open-telemetry/opentelemetry-cpp/pull/1427))
+
 ## [1.4.0] 2022-05-17
 
 * [API SDK] Upgrade proto to v0.17.0, update log data model ([#1383](https://github.com/open-telemetry/opentelemetry-cpp/pull/1383))

diff --git a/api/include/opentelemetry/trace/span_context.h b/api/include/opentelemetry/trace/span_context.h
@@ -81,7 +81,7 @@ class SpanContext final
 
   bool IsRemote() const noexcept { return is_remote_; }
 
-  static SpanContext GetInvalid() { return SpanContext(false, false); }
+  static SpanContext GetInvalid() noexcept { return SpanContext(false, false); }
 
   bool IsSampled() const noexcept { return trace_flags_.IsSampled(); }
 

diff --git a/exporters/etw/include/opentelemetry/exporters/etw/etw_logger_exporter.h b/exporters/etw/include/opentelemetry/exporters/etw/etw_logger_exporter.h
@@ -15,7 +15,7 @@
 
 #  include "opentelemetry/common/key_value_iterable_view.h"
 
-#  include "opentelemetry/logs/tracer_provider.h"
+#  include "opentelemetry/logs/logger_provider.h"
 #  include "opentelemetry/trace/span_id.h"
 #  include "opentelemetry/trace/trace_id.h"
 

diff --git a/exporters/etw/include/opentelemetry/exporters/etw/etw_properties.h b/exporters/etw/include/opentelemetry/exporters/etw/etw_properties.h
@@ -192,7 +192,7 @@ class PropertyValue : public PropertyVariant
   {}
 
   /**
-   * @brief Convert owning PropertyValue to non-owning common::AttributeValue
+   * @brief Convert non-owning common::AttributeValue to owning PropertyValue.
    * @return
    */
   PropertyValue &FromAttributeValue(const common::AttributeValue &v)
@@ -222,7 +222,8 @@ class PropertyValue : public PropertyVariant
         break;
       }
       case common::AttributeType::kTypeString: {
-        PropertyVariant::operator=(nostd::string_view(nostd::get<nostd::string_view>(v)).data());
+        PropertyVariant::operator=
+            (std::string{nostd::string_view(nostd::get<nostd::string_view>(v)).data()});
         break;
       }
 

diff --git a/exporters/etw/test/etw_logger_test.cc b/exporters/etw/test/etw_logger_test.cc
@@ -8,7 +8,7 @@
 #    include <map>
 #    include <string>
 
-#    include "opentelemetry/exporters/etw/etw_logger.h"
+#    include "opentelemetry/exporters/etw/etw_logger_exporter.h"
 #    include "opentelemetry/sdk/trace/simple_processor.h"
 
 using namespace OPENTELEMETRY_NAMESPACE;

diff --git a/exporters/jaeger/BUILD b/exporters/jaeger/BUILD
@@ -180,6 +180,7 @@ cc_library(
     tags = ["jaeger"],
     deps = [
         ":jaeger_exporter",
+        "//sdk/src/common:global_log_handler",
     ],
 )
 

diff --git a/sdk/include/opentelemetry/sdk/common/global_log_handler.h b/sdk/include/opentelemetry/sdk/common/global_log_handler.h
@@ -55,7 +55,7 @@ inline std::string LevelToString(LogLevel level)
 class LogHandler
 {
 public:
-  virtual ~LogHandler() = default;
+  virtual ~LogHandler();
 
   virtual void Handle(LogLevel level,
                       const char *file,
@@ -71,22 +71,7 @@ class DefaultLogHandler : public LogHandler
               const char *file,
               int line,
               const char *msg,
-              const sdk::common::AttributeMap &attributes) noexcept override
-  {
-    std::stringstream output_s;
-    output_s << "[" << LevelToString(level) << "] ";
-    if (file != nullptr)
-    {
-      output_s << "File: " << file << ":" << line;
-    }
-    if (msg != nullptr)
-    {
-      output_s << msg;
-    }
-    output_s << std::endl;
-    // TBD - print attributes
-    std::cout << output_s.str();  // thread safe.
-  }
+              const sdk::common::AttributeMap &attributes) noexcept override;
 };
 
 class NoopLogHandler : public LogHandler
@@ -96,10 +81,7 @@ class NoopLogHandler : public LogHandler
               const char *file,
               int line,
               const char *msg,
-              const sdk::common::AttributeMap &error_attributes) noexcept override
-  {
-    // ignore the log message
-  }
+              const sdk::common::AttributeMap &error_attributes) noexcept override;
 };
 
 /**
@@ -113,7 +95,7 @@ class GlobalLogHandler
    *
    * By default, a default LogHandler is returned.
    */
-  static const nostd::shared_ptr<LogHandler> &GetLogHandler() noexcept
+  static inline const nostd::shared_ptr<LogHandler> &GetLogHandler() noexcept
   {
     return GetHandlerAndLevel().first;
   }
@@ -123,7 +105,7 @@ class GlobalLogHandler
    * This should be called once at the start of application before creating any Provider
    * instance.
    */
-  static void SetLogHandler(nostd::shared_ptr<LogHandler> eh) noexcept
+  static inline void SetLogHandler(nostd::shared_ptr<LogHandler> eh) noexcept
   {
     GetHandlerAndLevel().first = eh;
   }
@@ -133,22 +115,17 @@ class GlobalLogHandler
    *
    * By default, a default log level is returned.
    */
-  static LogLevel GetLogLevel() noexcept { return GetHandlerAndLevel().second; }
+  static inline LogLevel GetLogLevel() noexcept { return GetHandlerAndLevel().second; }
 
   /**
    * Changes the singleton Log level.
    * This should be called once at the start of application before creating any Provider
    * instance.
    */
-  static void SetLogLevel(LogLevel level) noexcept { GetHandlerAndLevel().second = level; }
+  static inline void SetLogLevel(LogLevel level) noexcept { GetHandlerAndLevel().second = level; }
 
 private:
-  static std::pair<nostd::shared_ptr<LogHandler>, LogLevel> &GetHandlerAndLevel() noexcept
-  {
-    static std::pair<nostd::shared_ptr<LogHandler>, LogLevel> handler_and_level{
-        nostd::shared_ptr<LogHandler>(new DefaultLogHandler), LogLevel::Warning};
-    return handler_and_level;
-  }
+  static std::pair<nostd::shared_ptr<LogHandler>, LogLevel> &GetHandlerAndLevel() noexcept;
 };
 
 }  // namespace internal_log

diff --git a/sdk/src/common/BUILD b/sdk/src/common/BUILD
@@ -27,6 +27,18 @@ cc_library(
     include_prefix = "src/common",
     deps = [
         "//api",
+        "//sdk:headers",
         "//sdk/src/common/platform:fork",
     ],
 )
+
+cc_library(
+    name = "global_log_handler",
+    srcs = [
+        "global_log_handler.cc",
+    ],
+    deps = [
+        "//api",
+        "//sdk:headers",
+    ],
+)
diff --git a/sdk/src/common/CMakeLists.txt b/sdk/src/common/CMakeLists.txt
@@ -1,4 +1,4 @@
-set(COMMON_SRCS random.cc core.cc)
+set(COMMON_SRCS random.cc core.cc global_log_handler.cc)
 if(WIN32)
   list(APPEND COMMON_SRCS platform/fork_windows.cc)
 else()

diff --git a/sdk/src/common/global_log_handler.cc b/sdk/src/common/global_log_handler.cc
@@ -0,0 +1,57 @@
+// Copyright The OpenTelemetry Authors
+// SPDX-License-Identifier: Apache-2.0
+
+#include "opentelemetry/sdk/common/global_log_handler.h"
+
+#include <cstring>
+#include <random>
+
+OPENTELEMETRY_BEGIN_NAMESPACE
+namespace sdk
+{
+namespace common
+{
+namespace internal_log
+{
+
+LogHandler::~LogHandler() {}
+
+void DefaultLogHandler::Handle(LogLevel level,
+                               const char *file,
+                               int line,
+                               const char *msg,
+                               const sdk::common::AttributeMap &attributes) noexcept
+{
+  std::stringstream output_s;
+  output_s << "[" << LevelToString(level) << "] ";
+  if (file != nullptr)
+  {
+    output_s << "File: " << file << ":" << line;
+  }
+  if (msg != nullptr)
+  {
+    output_s << msg;
+  }
+  output_s << std::endl;
+  // TBD - print attributes
+  std::cout << output_s.str();  // thread safe.
+}
+
+void NoopLogHandler::Handle(LogLevel,
+                            const char *,
+                            int,
+                            const char *,
+                            const sdk::common::AttributeMap &) noexcept
+{}
+
+std::pair<nostd::shared_ptr<LogHandler>, LogLevel> &GlobalLogHandler::GetHandlerAndLevel() noexcept
+{
+  static std::pair<nostd::shared_ptr<LogHandler>, LogLevel> handler_and_level{
+      nostd::shared_ptr<LogHandler>(new DefaultLogHandler), LogLevel::Warning};
+  return handler_and_level;
+}
+
+}  // namespace internal_log
+}  // namespace common
+}  // namespace sdk
+OPENTELEMETRY_END_NAMESPACE
diff --git a/sdk/src/logs/BUILD b/sdk/src/logs/BUILD
@@ -22,6 +22,7 @@ cc_library(
     deps = [
         "//api",
         "//sdk:headers",
+        "//sdk/src/common:global_log_handler",
         "//sdk/src/resource",
     ],
 )
diff --git a/sdk/src/metrics/BUILD b/sdk/src/metrics/BUILD
@@ -22,6 +22,7 @@ cc_library(
     deps = [
         "//api",
         "//sdk:headers",
+        "//sdk/src/common:global_log_handler",
         "//sdk/src/resource",
     ],
 )
diff --git a/sdk/src/metrics/sync_instruments.cc b/sdk/src/metrics/sync_instruments.cc
@@ -4,6 +4,9 @@
 #ifndef ENABLE_METRICS_PREVIEW
 #  include "opentelemetry/sdk/metrics/sync_instruments.h"
 #  include "opentelemetry/sdk/metrics/state/metric_storage.h"
+#  include "opentelemetry/sdk_config.h"
+
+#  include <cmath>
 
 OPENTELEMETRY_BEGIN_NAMESPACE
 namespace sdk
@@ -139,11 +142,25 @@ void LongHistogram::Record(long value,
                            const opentelemetry::common::KeyValueIterable &attributes,
                            const opentelemetry::context::Context &context) noexcept
 {
+  if (value < 0)
+  {
+    OTEL_INTERNAL_LOG_WARN(
+        "[LongHistogram::Record(value, attributes)] negative value provided to histogram Name:"
+        << instrument_descriptor_.name_ << " Value:" << value);
+    return;
+  }
   return storage_->RecordLong(value, attributes, context);
 }
 
 void LongHistogram::Record(long value, const opentelemetry::context::Context &context) noexcept
 {
+  if (value < 0)
+  {
+    OTEL_INTERNAL_LOG_WARN(
+        "[LongHistogram::Record(value)] negative value provided to histogram Name:"
+        << instrument_descriptor_.name_ << " Value:" << value);
+    return;
+  }
   return storage_->RecordLong(value, context);
 }
 
@@ -156,11 +173,26 @@ void DoubleHistogram::Record(double value,
                              const opentelemetry::common::KeyValueIterable &attributes,
                              const opentelemetry::context::Context &context) noexcept
 {
+  if (value < 0 || std::isnan(value) || std::isinf(value))
+  {
+    OTEL_INTERNAL_LOG_WARN(
+        "[DoubleHistogram::Record(value, attributes)] negative/nan/infinite value provided to "
+        "histogram Name:"
+        << instrument_descriptor_.name_);
+    return;
+  }
   return storage_->RecordDouble(value, attributes, context);
 }
 
 void DoubleHistogram::Record(double value, const opentelemetry::context::Context &context) noexcept
 {
+  if (value < 0 || std::isnan(value) || std::isinf(value))
+  {
+    OTEL_INTERNAL_LOG_WARN(
+        "[DoubleHistogram::Record(value)] negative/nan/infinite value provided to histogram Name:"
+        << instrument_descriptor_.name_);
+    return;
+  }
   return storage_->RecordDouble(value, context);
 }
 

diff --git a/sdk/src/trace/BUILD b/sdk/src/trace/BUILD
@@ -22,6 +22,7 @@ cc_library(
     deps = [
         "//api",
         "//sdk:headers",
+        "//sdk/src/common:global_log_handler",
         "//sdk/src/common:random",
         "//sdk/src/resource",
     ],

diff --git a/sdk/test/common/BUILD b/sdk/test/common/BUILD
@@ -124,6 +124,7 @@ cc_test(
     deps = [
         "//api",
         "//sdk:headers",
+        "//sdk/src/common:global_log_handler",
         "@com_google_googletest//:gtest_main",
     ],
 )

diff --git a/sdk/test/metrics/sync_instruments_test.cc b/sdk/test/metrics/sync_instruments_test.cc
@@ -9,6 +9,8 @@
 #  include "opentelemetry/sdk/metrics/state/multi_metric_storage.h"
 
 #  include <gtest/gtest.h>
+#  include <cmath>
+#  include <limits>
 
 using namespace opentelemetry;
 using namespace opentelemetry::sdk::instrumentationlibrary;
@@ -103,7 +105,7 @@ TEST(SyncInstruments, LongHistogram)
   std::unique_ptr<WritableMetricStorage> metric_storage(new MultiMetricStorage());
   LongHistogram counter(instrument_descriptor, std::move(metric_storage));
   EXPECT_NO_THROW(counter.Record(10l, opentelemetry::context::Context{}));
-  EXPECT_NO_THROW(counter.Record(10l, opentelemetry::context::Context{}));
+  EXPECT_NO_THROW(counter.Record(-10l, opentelemetry::context::Context{}));  // This is ignored
 
   EXPECT_NO_THROW(counter.Record(
       10l, opentelemetry::common::KeyValueIterableView<M>({{"abc", "123"}, {"xyz", "456"}}),
@@ -120,7 +122,11 @@ TEST(SyncInstruments, DoubleHistogram)
   std::unique_ptr<WritableMetricStorage> metric_storage(new MultiMetricStorage());
   DoubleHistogram counter(instrument_descriptor, std::move(metric_storage));
   EXPECT_NO_THROW(counter.Record(10.10, opentelemetry::context::Context{}));
-  EXPECT_NO_THROW(counter.Record(10.10, opentelemetry::context::Context{}));
+  EXPECT_NO_THROW(counter.Record(-10.10, opentelemetry::context::Context{}));  // This is ignored.
+  EXPECT_NO_THROW(counter.Record(std::numeric_limits<double>::quiet_NaN(),
+                                 opentelemetry::context::Context{}));  // This is ignored too
+  EXPECT_NO_THROW(counter.Record(std::numeric_limits<double>::infinity(),
+                                 opentelemetry::context::Context{}));  // This is ignored too
 
   EXPECT_NO_THROW(counter.Record(
       10.10, opentelemetry::common::KeyValueIterableView<M>({{"abc", "123"}, {"xyz", "456"}}),