ASSUME and DEBUG_ASSUME macros. (#226)

ASSUME is now enabled in both debug and release builds. DEBUG_ASSUME is enabled only in debug builds.
open-telemetry · Jul 12, 2023 · 8f04e6d · 8f04e6d
1 parent f5a6fae
commit 8f04e6d
Show file tree

Hide file tree

Showing 9 changed files with 28 additions and 28 deletions.
diff --git a/channel/tcp_channel.cc b/channel/tcp_channel.cc
@@ -74,7 +74,7 @@ void TCPChannel::conn_read_cb(uv_stream_t *stream, ssize_t nread, const uv_buf_t
     u32 res = conn->callbacks_->received_data((u8 *)conn->rx_buffer_, conn->rx_len_);
 
     if (res > 0) {
-      ASSUME(res <= conn->rx_len_);
+      DEBUG_ASSUME(res <= conn->rx_len_);
       conn->rx_len_ -= res;
       memmove(conn->rx_buffer_, (u8 *)conn->rx_buffer_ + res, conn->rx_len_);
     }
@@ -163,7 +163,7 @@ TCPChannel::TCPChannel(uv_loop_t &loop, std::string addr, std::string port) : ad
 TCPChannel::~TCPChannel()
 {
   LOG::trace_in(channel::Component::tcp, "TCPChannel::{}: connection not closing, calling close on handle()", __func__);
-  ASSUME(uv_is_closing((uv_handle_t *)&conn_));
+  DEBUG_ASSUME(uv_is_closing((uv_handle_t *)&conn_));
 }
 
 void TCPChannel::connect(Callbacks &callbacks)

diff --git a/collector/kernel/kernel_collector_test.cc b/collector/kernel/kernel_collector_test.cc
@@ -513,7 +513,7 @@ TEST_F(KernelCollectorTest, bpf_log)
   auto ingest_msg_cb = [&](nlohmann::json const &object) {
     SCOPED_TIMING(BpfLogTestIngestMsgCb);
     // Fail immediately if a bpf_log is encountered.
-    ASSUME(object["name"] != "bpf_log").else_log("got bpf_log {}", to_string(object));
+    DEBUG_ASSUME(object["name"] != "bpf_log").else_log("got bpf_log {}", to_string(object));
   };
 
   start_kernel_collector(IntakeEncoder::binary, stop_conditions, BPF_DUMP_FILE, ingest_msg_cb);

diff --git a/reducer/aggregation/agg_root_span.cc b/reducer/aggregation/agg_root_span.cc
@@ -38,7 +38,7 @@ AggRootSpan::~AggRootSpan() {}
 void AggRootSpan::update_node(
     ::ebpf_net::aggregation::weak_refs::agg_root span_ref, u64 timestamp, jsrv_aggregation__update_node *msg)
 {
-  ASSUME(msg->side <= 1).else_log("side must be either 0 or 1, instead got {}", msg->side);
+  DEBUG_ASSUME(msg->side <= 1).else_log("side must be either 0 or 1, instead got {}", msg->side);
 
   auto &stat_counters = local_core<AggCore>().stat_counters;
 

diff --git a/reducer/ingest/ingest_core.cc b/reducer/ingest/ingest_core.cc
@@ -71,10 +71,10 @@ IngestCore::IngestCore(
   }
   stop_async_.data = this;
 
-  ASSUME(ingest_to_logging_queues.num_receivers() == 1).else_log("Not using multiple logging cores");
+  DEBUG_ASSUME(ingest_to_logging_queues.num_receivers() == 1).else_log("Not using multiple logging cores");
 
   // Create the ingest workers and start the telemetry TCP server.
-  ASSUME(ingest_shard_count > 0).else_log("Ingest shards should be > 0, instead got {}", ingest_shard_count);
+  DEBUG_ASSUME(ingest_shard_count > 0).else_log("Ingest shards should be > 0, instead got {}", ingest_shard_count);
 
   std::vector<std::unique_ptr<IngestWorker>> workers;
   workers.reserve(ingest_shard_count);

diff --git a/reducer/ingest/ingest_worker.cc b/reducer/ingest/ingest_worker.cc
@@ -130,7 +130,7 @@ uint32_t IngestWorker::Callbacks::received_data(const u8 *data, int data_len)
     }
 
     // Increment the begin pointer by the bytes consumed.
-    ASSUME(static_cast<int>(*bytes_consumed) <= (end - begin));
+    DEBUG_ASSUME(static_cast<int>(*bytes_consumed) <= (end - begin));
     begin += *bytes_consumed;
     ++count;
 

diff --git a/reducer/matching/flow_span.cc b/reducer/matching/flow_span.cc
@@ -695,7 +695,7 @@ void FlowSpan::http_update(::ebpf_net::matching::weak_refs::flow span_ref, u64 t
 
 void FlowSpan::dns_update(::ebpf_net::matching::weak_refs::flow span_ref, u64 timestamp, jsrv_matching__dns_update *msg)
 {
-  ASSUME(msg->side <= 1).else_log("side must be either 0 or 1, instead got {}", msg->side);
+  DEBUG_ASSUME(msg->side <= 1).else_log("side must be either 0 or 1, instead got {}", msg->side);
 
   auto const side = u8_to_side(msg->side);
   auto const is_rx = msg->client_server == (u8)SC_SERVER; // client(0) is 'tx', server(1) is 'rx'

diff --git a/reducer/otlp_grpc_formatter.cc b/reducer/otlp_grpc_formatter.cc
@@ -83,7 +83,7 @@ void OtlpGrpcFormatter::format(
 
   auto print_ascii = [&](const std::string &str) { return is_non_ascii(str) ? "<ERROR_NON_ASCII>" : str; };
 
-  ASSUME(!metric_info.name.empty()).else_log("empty metric name");
+  DEBUG_ASSUME(!metric_info.name.empty()).else_log("empty metric name");
   bool found_non_ascii = false;
   found_non_ascii |= is_non_ascii(metric_info.name);
   found_non_ascii |= is_non_ascii(metric_info.unit);
@@ -93,7 +93,7 @@ void OtlpGrpcFormatter::format(
       throw std::invalid_argument(fmt::format("empty label key for metric={}", metric_info.name));
     }
     found_non_ascii |= is_non_ascii(key);
-    ASSUME(!(key != "span" && key != "error" && value.empty()))
+    DEBUG_ASSUME(!(key != "span" && key != "error" && value.empty()))
         .else_log("empty label value for metric={} label={}", metric_info.name, key);
     found_non_ascii |= is_non_ascii(value);
   }
@@ -118,7 +118,7 @@ void OtlpGrpcFormatter::format(
       }
       metric_string += " }";
     }
-    ASSUME(!found_non_ascii).else_log("OtlpGrpcFormatter detected non-ascii character(s) in metric: {}", metric_string);
+    DEBUG_ASSUME(!found_non_ascii).else_log("OtlpGrpcFormatter detected non-ascii character(s) in metric: {}", metric_string);
   }
 #endif
 

diff --git a/util/error_handling.cc b/util/error_handling.cc
@@ -16,8 +16,6 @@
 
 namespace {
 
-#ifndef NDEBUG
-
 void stacktrace(std::ostream &os)
 {
   // Get the list of PCs
@@ -69,6 +67,4 @@ Panicker::~Panicker()
   std::exit(1);
 }
 
-#endif // NDEBUG
-
 } // namespace internal
diff --git a/util/error_handling.h b/util/error_handling.h
@@ -34,14 +34,22 @@
 //
 // None of the expressions in the custom message are evaluated if the
 // conditional evaluates to true.
-
-namespace internal {
-
-#ifndef NDEBUG
-
+//
 #define ASSUME(...)                                                                                                            \
   (__VA_ARGS__) || ::internal::Panicker(::absl::StrCat("Assumption `", #__VA_ARGS__, "` failed at ", __FILE__, ":", __LINE__))
 
+// DEBUG_ASSUME - version of the ASSUME macro enabled only on debug builds.
+// To be used when the evaluation of the provided expression is too costly for
+// release builds.
+//
+#ifdef NDEBUG
+#define DEBUG_ASSUME(...) (::internal::NopPanicker{})
+#else
+#define DEBUG_ASSUME ASSUME
+#endif // NDEBUG
+
+namespace internal {
+
 // Internal class that prints a message. Exits and dumps everything in the
 // destructor.
 struct Panicker {
@@ -73,14 +81,10 @@ struct Panicker {
   std::string custom_message_;
 };
 
-#else // NDEBUG
-#define ASSUME(...) (::internal::Panicker{})
-
-struct Panicker {
-  // Custom message formatter. Uses spdlog-like formatting.
-  template <typename... Args> Panicker &else_log(Args &&... args) { return *this; }
-
-  // Needed so that the `(expression) || Panicker(...)` statement compiles.
+#ifdef NDEBUG
+// Dummy panicker used by DEBUG_ASSUME on non-debug builds.
+struct NopPanicker {
+  template <typename... Args> NopPanicker &else_log(Args &&... args) { return *this; }
   operator bool() const { return true; }
 };
 #endif // NDEBUG