Merge pull request ClickHouse#79879 from ClickHouse/always-log-important-errors

Always log critical exceptions

Author: mstetsyuk

src/Common/Exception.cpp

@@ -17,6 +17,7 @@
 #include <Common/logger_useful.h>
 
 #include <algorithm>
+#include <atomic>
 #include <cstdlib>
 #include <cstring>
 #include <filesystem>
@@ -37,6 +38,7 @@ namespace ErrorCodes
     extern const int LOGICAL_ERROR;
     extern const int CANNOT_ALLOCATE_MEMORY;
     extern const int CANNOT_MREMAP;
+    extern const int POTENTIALLY_BROKEN_DATA_PART;
 }
 
 void abortOnFailedAssertion(const String & description, void * const * trace, size_t trace_offset, size_t trace_size)
@@ -263,6 +265,25 @@ void Exception::clearThreadFramePointers()
         thread_frame_pointers.frame_pointers.clear();
 }
 
+bool Exception::isErrorCodeImportant() const
+{
+    const int error_code = code();
+    return error_code == ErrorCodes::LOGICAL_ERROR
+        || error_code == ErrorCodes::POTENTIALLY_BROKEN_DATA_PART;
+}
+
+Exception::~Exception()
+try
+{
+    if (logged != nullptr && !logged->load(std::memory_order_relaxed) && isErrorCodeImportant() && isLoggingEnabled())
+    {
+        LOG_ERROR(getLogger("ForcedCriticalErrorsLogger"), "{}", getExceptionMessage(*this, /*with_stacktrace=*/ true));
+    }
+}
+catch (...) // NOLINT(bugprone-empty-catch)
+{
+}
+
 static void tryLogCurrentExceptionImpl(Poco::Logger * logger, const std::string & start_of_message, LogsLevel level)
 {
     if (!isLoggingEnabled())
@@ -290,6 +311,19 @@ static void tryLogCurrentExceptionImpl(Poco::Logger * logger, const std::string
     catch (...) // NOLINT(bugprone-empty-catch)
     {
     }
+
+    /// Mark the exception as logged.
+    try
+    {
+        throw;
+    }
+    catch (Exception & e)
+    {
+        e.markAsLogged();
+    }
+    catch (...) // NOLINT(bugprone-empty-catch)
+    {
+    }
 }
 
 void tryLogCurrentException(const char * log_name, const std::string & start_of_message, LogsLevel level)

src/Common/Exception.h

@@ -6,6 +6,7 @@
 #include <Common/StackTrace.h>
 #include <Core/LogsLevel.h>
 
+#include <atomic>
 #include <cerrno>
 #include <exception>
 #include <vector>
@@ -62,6 +63,12 @@ class Exception : public Poco::Exception
         message_format_string_args = msg.format_string_args;
     }
 
+    ~Exception() override;
+    Exception(const Exception &) = default;
+    Exception & operator=(const Exception &) = default;
+    Exception(Exception &&) = default;
+    Exception & operator=(Exception &&) = default;
+
     /// Collect call stacks of all previous jobs' schedulings leading to this thread job's execution
     static thread_local bool enable_job_stack_trace;
     using ThreadFramePointersBase = std::vector<StackTrace::FramePointers>;
@@ -166,11 +173,23 @@ class Exception : public Poco::Exception
 
     std::vector<std::string> getMessageFormatStringArgs() const { return message_format_string_args; }
 
+    void markAsLogged()
+    {
+        if (logged)
+        {
+            logged->store(true, std::memory_order_relaxed);
+        }
+    }
+
+    /// Indicates if the error code triggers alerts in ClickHouse Cloud
+    bool isErrorCodeImportant() const;
+
 private:
 #ifndef STD_EXCEPTION_HAS_STACK_TRACE
     StackTrace trace;
 #endif
     bool remote = false;
+    std::shared_ptr<std::atomic<bool>> logged = std::make_shared<std::atomic<bool>>(false);
 
     /// Number of this error among other errors with the same code and the same `remote` flag since the program startup.
     size_t error_index = static_cast<size_t>(-1);