From c3d91f519098c8a5145a17faee04aeb4deacfb60 Mon Sep 17 00:00:00 2001
From: Kefu Chai <kefu.chai@scylladb.com>
Date: Fri, 9 Jun 2023 12:47:19 +0800
Subject: [PATCH] tracing: drop trace(.., std::string&&) overload

this change is a follow-up of 4f5fcb02fdf40244b13f908334081212c581a1e6,
the goal is to avoid the programming oversights like

```c++
trace(trace_ptr, "foo {} with {} but {} is {}");
```

as `trace(const trace_state_ptr& p, const std::string& msg)` is
a better match than the templated one, i.e.,
`trace(const trace_state_ptr& p, fmt::format_string<T...> fmt, T&&...
args)`. so we cannot detect this with the compile-time format checking.

so let's just drop this overload, and update its callers to use
the other overload.

The change was suggested by Avi. the example also came from him.

Signed-off-by: Kefu Chai <kefu.chai@scylladb.com>

Closes #14188
---
 alternator/server.cc   |  2 +-
 tracing/trace_state.hh | 15 ---------------
 transport/server.cc    |  2 +-
 3 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/alternator/server.cc b/alternator/server.cc
index d4843f0fa3..c6131c0836 100644
--- a/alternator/server.cc
+++ b/alternator/server.cc
@@ -424,7 +424,7 @@ future<executor::request_return_type> server::handle_api_request(std::unique_ptr
     co_await client_state.maybe_update_per_service_level_params();
 
     tracing::trace_state_ptr trace_state = maybe_trace_query(client_state, username, op, content);
-    tracing::trace(trace_state, std::move(op));
+    tracing::trace(trace_state, "{}", op);
     rjson::value json_request = co_await _json_parser.parse(std::move(content));
     co_return co_await callback_it->second(_executor, client_state, trace_state,
             make_service_permit(std::move(units)), std::move(json_request), std::move(req));
diff --git a/tracing/trace_state.hh b/tracing/trace_state.hh
index 016c5421d7..873068c46e 100644
--- a/tracing/trace_state.hh
+++ b/tracing/trace_state.hh
@@ -454,15 +454,6 @@ private:
         }
     }
 
-    void trace(const char* msg) noexcept {
-        try {
-            trace_internal(std::string(msg));
-        } catch (...) {
-            // Bump up an error counter and ignore
-            ++_local_tracing_ptr->stats.trace_errors;
-        }
-    }
-
     /**
      * Add a single trace entry - printf-like version
      *
@@ -716,12 +707,6 @@ inline void trace(const trace_state_ptr& p, fmt::format_string<T...> fmt, T&&...
     }
 }
 
-inline void trace(const trace_state_ptr& p, std::string&& msg) noexcept {
-    if (p && !p->ignore_events()) {
-        p->trace(std::move(msg));
-    }
-}
-
 inline std::optional<trace_info> make_trace_info(const trace_state_ptr& state) {
     // We want to trace the remote replicas' operations only when a full tracing
     // is requested or when a slow query logging is enabled and the session is
diff --git a/transport/server.cc b/transport/server.cc
index 34f76fa42a..c00d651ec1 100644
--- a/transport/server.cc
+++ b/transport/server.cc
@@ -1099,7 +1099,7 @@ process_execute_internal(service::client_state& client_state, distributed<cql3::
         const auto msg = format("Invalid amount of bind variables: expected {:d} received {:d}",
                 stmt->get_bound_terms(),
                 options.get_values_count());
-        tracing::trace(query_state.get_trace_state(), msg);
+        tracing::trace(query_state.get_trace_state(), "{}", msg);
         throw exceptions::invalid_request_exception(msg);
     }