From 0e77692a278b818d03bfd0987c7115802f89a648 Mon Sep 17 00:00:00 2001 From: Ivan Blinkov Date: Tue, 16 Jun 2020 15:56:28 +0300 Subject: [PATCH] improvements after review comments --- base/daemon/SentryWriter.cpp | 32 ++++++++++--------- base/daemon/SentryWriter.h | 10 +++++- .../settings.md | 2 +- programs/server/config.xml | 3 +- 4 files changed, 29 insertions(+), 18 deletions(-) diff --git a/base/daemon/SentryWriter.cpp b/base/daemon/SentryWriter.cpp index 88639d8bf94..0524285ea42 100644 --- a/base/daemon/SentryWriter.cpp +++ b/base/daemon/SentryWriter.cpp @@ -14,6 +14,7 @@ #if USE_SENTRY # include // Y_IGNORE # include +# include #endif @@ -31,7 +32,7 @@ void setExtras() { sentry_set_extra("server_name", sentry_value_new_string(getFQDNOrHostName().c_str())); } - sentry_set_tag("version", VERSION_STRING_SHORT); + sentry_set_tag("version", VERSION_STRING); sentry_set_extra("version_githash", sentry_value_new_string(VERSION_GITHASH)); sentry_set_extra("version_describe", sentry_value_new_string(VERSION_DESCRIBE)); sentry_set_extra("version_integer", sentry_value_new_int32(VERSION_INTEGER)); @@ -93,14 +94,15 @@ void SentryWriter::initialize(Poco::Util::LayeredConfiguration & config) } if (enabled) { + const std::filesystem::path & default_tmp_path = std::filesystem::path(config.getString("tmp_path", Poco::Path::temp())) / "sentry"; const std::string & endpoint = config.getString("send_crash_reports.endpoint", "https://6f33034cfe684dd7a3ab9875e57b1c8d@o388870.ingest.sentry.io/5226277"); const std::string & temp_folder_path - = config.getString("send_crash_reports.tmp_path", config.getString("tmp_path", Poco::Path::temp()) + "sentry/"); + = config.getString("send_crash_reports.tmp_path", default_tmp_path); Poco::File(temp_folder_path).createDirectories(); - sentry_options_t * options = sentry_options_new(); - sentry_options_set_release(options, VERSION_STRING); + sentry_options_t * options = sentry_options_new(); /// will be freed by sentry_init or sentry_shutdown + sentry_options_set_release(options, VERSION_STRING_SHORT); sentry_options_set_logger(options, &sentry_logger); if (debug) { @@ -128,17 +130,16 @@ void SentryWriter::initialize(Poco::Util::LayeredConfiguration & config) { initialized = true; anonymize = config.getBool("send_crash_reports.anonymize", false); - const std::string& anonymize_status = anonymize ? " (anonymized)" : ""; LOG_INFO( logger, "Sending crash reports is initialized with {} endpoint and {} temp folder{}", endpoint, temp_folder_path, - anonymize_status); + anonymize ? " (anonymized)" : ""); } else { - LOG_WARNING(logger, "Sending crash reports failed to initialized with {} status", init_status); + LOG_WARNING(logger, "Sending crash reports failed to initialize with {} status", init_status); } } else @@ -177,21 +178,20 @@ void SentryWriter::onFault(int sig, const siginfo_t & info, const ucontext_t & c size_t stack_size = stack_trace.getSize(); if (stack_size > 0) { - size_t offset = stack_trace.getOffset(); - if (stack_size == 1) - { - offset = 1; - } + ssize_t offset = stack_trace.getOffset(); char instruction_addr[100]; StackTrace::Frames frames; StackTrace::symbolize(stack_trace.getFramePointers().data(), offset, stack_size, frames); - for (size_t i = stack_size - 1; i >= offset; --i) + for (ssize_t i = stack_size - 1; i >= offset; --i) { const StackTrace::Frame & current_frame = frames[i]; sentry_value_t sentry_frame = sentry_value_new_object(); UInt64 frame_ptr = reinterpret_cast(current_frame.virtual_addr); - std::snprintf(instruction_addr, sizeof(instruction_addr), "0x%" PRIx64, frame_ptr); - sentry_value_set_by_key(sentry_frame, "instruction_addr", sentry_value_new_string(instruction_addr)); + + if (std::snprintf(instruction_addr, sizeof(instruction_addr), "0x%" PRIx64, frame_ptr) >= 0) + { + sentry_value_set_by_key(sentry_frame, "instruction_addr", sentry_value_new_string(instruction_addr)); + } if (current_frame.symbol.has_value()) { @@ -213,6 +213,7 @@ void SentryWriter::onFault(int sig, const siginfo_t & info, const ucontext_t & c } /// Prepare data for https://develop.sentry.dev/sdk/event-payloads/threads/ + /// Stacktrace is filled only for a single thread that failed sentry_value_t stacktrace = sentry_value_new_object(); sentry_value_set_by_key(stacktrace, "frames", sentry_frames); @@ -225,6 +226,7 @@ void SentryWriter::onFault(int sig, const siginfo_t & info, const ucontext_t & c sentry_value_t threads = sentry_value_new_object(); sentry_value_set_by_key(threads, "values", values); + sentry_value_set_by_key(event, "threads", threads); LOG_INFO(logger, "Sending crash report"); diff --git a/base/daemon/SentryWriter.h b/base/daemon/SentryWriter.h index 0b3f1ddd2b7..655a4e93bfd 100644 --- a/base/daemon/SentryWriter.h +++ b/base/daemon/SentryWriter.h @@ -7,7 +7,13 @@ #include -/// Sends crash reports to ClickHouse core developer team via https://sentry.io +/// \brief Sends crash reports to ClickHouse core developer team via https://sentry.io +/// +/// This feature can enabled with "send_crash_reports.enabled" server setting, +/// in this case reports are sent only for official ClickHouse builds. +/// +/// It is possible to send those reports to your own sentry account or account of consulting company you hired +/// by overriding "send_crash_reports.endpoint" setting. "send_crash_reports.debug" setting will allow to do that for class SentryWriter { public: @@ -15,6 +21,8 @@ public: static void initialize(Poco::Util::LayeredConfiguration & config); static void shutdown(); + + /// Not signal safe and can't be called from a signal handler static void onFault( int sig, const siginfo_t & info, diff --git a/docs/en/operations/server-configuration-parameters/settings.md b/docs/en/operations/server-configuration-parameters/settings.md index e1ff3a872d1..58a02b8266a 100644 --- a/docs/en/operations/server-configuration-parameters/settings.md +++ b/docs/en/operations/server-configuration-parameters/settings.md @@ -353,7 +353,7 @@ Keys: Settings for opt-in sending crash reports to the ClickHouse core developers team via [Sentry](https://sentry.io). Enabling it, especially in pre-production environments, is greatly appreciated. -The server will need an access to public Internet for this feature to be functioning properly. +The server will need an access to public Internet via IPv4 (at the time of writing IPv6 is not supported by Sentry) for this feature to be functioning properly. Keys: diff --git a/programs/server/config.xml b/programs/server/config.xml index afb44989bbe..e5482a074a3 100644 --- a/programs/server/config.xml +++ b/programs/server/config.xml @@ -46,7 +46,8 @@ - + + false