From 2fb49503dacee99091bb3804a71006394440e125 Mon Sep 17 00:00:00 2001
From: Frank Chen <frank.chen021@outlook.com>
Date: Thu, 19 Jan 2023 14:02:06 +0800
Subject: [PATCH] Make addAttribute exception safe

---
 src/Common/OpenTelemetryTraceContext.cpp | 71 ++++++++++--------------
 src/Common/OpenTelemetryTraceContext.h   | 19 +++----
 2 files changed, 39 insertions(+), 51 deletions(-)

diff --git a/src/Common/OpenTelemetryTraceContext.cpp b/src/Common/OpenTelemetryTraceContext.cpp
index 515060803d6..365725c1a98 100644
--- a/src/Common/OpenTelemetryTraceContext.cpp
+++ b/src/Common/OpenTelemetryTraceContext.cpp
@@ -14,76 +14,65 @@ namespace OpenTelemetry
 
 thread_local TracingContextOnThread current_thread_trace_context;
 
-void Span::addAttribute(std::string_view name, UInt64 value)
+bool Span::addAttribute(std::string_view name, UInt64 value) noexcept
 {
     if (!this->isTraceEnabled() || name.empty())
-        return;
+        return false;
 
-    this->attributes.push_back(Tuple{name, toString(value)});
+    return addAttribute(name, toString(value));
 }
 
-void Span::addAttributeIfNotZero(std::string_view name, UInt64 value)
+bool Span::addAttributeIfNotZero(std::string_view name, UInt64 value) noexcept
 {
-    if (value != 0)
-        addAttribute(name, value);
+    if (!this->isTraceEnabled() || name.empty() || value == 0)
+        return false;
+
+    return addAttribute(name, toString(value));
 }
 
-void Span::addAttribute(std::string_view name, std::string_view value)
+bool Span::addAttribute(std::string_view name, std::string_view value) noexcept
 {
     if (!this->isTraceEnabled() || name.empty())
-        return;
+        return false;
 
-    this->attributes.push_back(Tuple{name, value});
+    try
+    {
+        this->attributes.push_back(Tuple{name, value});
+    }
+    catch (...)
+    {
+        return false;
+    }
+    return true;
 }
 
-void Span::addAttributeIfNotEmpty(std::string_view name, std::string_view value)
+bool Span::addAttributeIfNotEmpty(std::string_view name, std::string_view value) noexcept
 {
-    if (!this->isTraceEnabled() || name.empty() || value.empty())
-        return;
-
-    this->attributes.push_back(Tuple{name, value});
+    return value.empty() ? false : addAttribute(name, value);
 }
 
-void Span::addAttribute(std::string_view name, std::function<String()> value_supplier)
+bool Span::addAttribute(std::string_view name, std::function<String()> value_supplier) noexcept
 {
     if (!this->isTraceEnabled() || !value_supplier)
-        return;
+        return false;
 
-    String value = value_supplier();
-    if (value.empty())
-        return;
-
-    this->attributes.push_back(Tuple{name, value});
+    return addAttributeIfNotEmpty(name, value_supplier());
 }
 
-void Span::addAttribute(const Exception & e) noexcept
+bool Span::addAttribute(const Exception & e) noexcept
 {
     if (!this->isTraceEnabled())
-        return;
+        return false;
 
-    try
-    {
-        this->attributes.push_back(Tuple{"clickhouse.exception", getExceptionMessage(e, false)});
-    }
-    catch (...)
-    {
-        /// Ignore exceptions
-    }
+    return addAttribute("clickhouse.exception", getExceptionMessage(e, false));
 }
 
-void Span::addAttribute(std::exception_ptr e) noexcept
+bool Span::addAttribute(std::exception_ptr e) noexcept
 {
     if (!this->isTraceEnabled() || e == nullptr)
-        return;
+        return false;
 
-    try
-    {
-        this->attributes.push_back(Tuple{"clickhouse.exception", getExceptionMessage(e, false)});
-    }
-    catch (...)
-    {
-        /// Ignore exceptions
-    }
+    return addAttribute("clickhouse.exception", getExceptionMessage(e, false));
 }
 
 SpanHolder::SpanHolder(std::string_view _operation_name)
diff --git a/src/Common/OpenTelemetryTraceContext.h b/src/Common/OpenTelemetryTraceContext.h
index 03bac2891fc..930262f2c92 100644
--- a/src/Common/OpenTelemetryTraceContext.h
+++ b/src/Common/OpenTelemetryTraceContext.h
@@ -23,16 +23,15 @@ struct Span
     UInt64 finish_time_us = 0;
     Map attributes;
 
-    void addAttribute(std::string_view name, UInt64 value);
-    void addAttributeIfNotZero(std::string_view name, UInt64 value);
-    void addAttribute(std::string_view name, std::string_view value);
-    void addAttributeIfNotEmpty(std::string_view name, std::string_view value);
-    void addAttribute(std::string_view name, std::function<String()> value_supplier);
-
-    /// Following two methods are declared as noexcept to make sure they're exception safe
-    /// This is because they're usually called in exception handler
-    void addAttribute(const Exception & e) noexcept;
-    void addAttribute(std::exception_ptr e) noexcept;
+    /// Following methods are declared as noexcept to make sure they're exception safe
+    /// This is because sometimes they will be called in exception handlers/dtor
+    bool addAttribute(std::string_view name, UInt64 value) noexcept;
+    bool addAttributeIfNotZero(std::string_view name, UInt64 value) noexcept;
+    bool addAttribute(std::string_view name, std::string_view value) noexcept;
+    bool addAttributeIfNotEmpty(std::string_view name, std::string_view value) noexcept;
+    bool addAttribute(std::string_view name, std::function<String()> value_supplier) noexcept;
+    bool addAttribute(const Exception & e) noexcept;
+    bool addAttribute(std::exception_ptr e) noexcept;
 
     bool isTraceEnabled() const
     {