Fix a couple thread-safety issues (#10359)

* Make CodeProbeImpl::_hitCount atomic

* Structure access to TraceLog::logTraceEventMetrics so that it is written before a trace log is opened and only read from one thread after it is opened.

* Fix condition in assert

* Rename TraceLog::log to logMetrics and move initialization of trace log metrics into TraceLog::open

---------

Co-authored-by: A.J. Beamon <aj.beamon@snowflake.com>

fdbclient/NativeAPI.actor.cpp

@@ -2313,7 +2313,8 @@ void initializeClientTracing(Reference<IClusterConnectionRecord> connRecord, Opt
 		              "trace",
 		              networkOptions.traceLogGroup,
 		              identifier,
-		              networkOptions.tracePartialFileSuffix);
+		              networkOptions.tracePartialFileSuffix,
+		              InitializeTraceMetrics::True);
 
 		TraceEvent("ClientStart")
 		    .detail("SourceVersion", getSourceVersion())
@@ -2328,7 +2329,6 @@ void initializeClientTracing(Reference<IClusterConnectionRecord> connRecord, Opt
 
 		g_network->initMetrics();
 		FlowTransport::transport().initMetrics();
-		initTraceEventMetrics();
 	}
 
 	// Initialize system monitoring once the local IP is available

fdbserver/fdbserver.actor.cpp

@@ -2044,8 +2044,16 @@ int main(int argc, char* argv[]) {
 				}
 			}
 
-			openTraceFile(
-			    opts.publicAddresses.address, opts.rollsize, opts.maxLogsSize, opts.logFolder, "trace", opts.logGroup);
+			openTraceFile(opts.publicAddresses.address,
+			              opts.rollsize,
+			              opts.maxLogsSize,
+			              opts.logFolder,
+			              "trace",
+			              opts.logGroup,
+			              /* identifier = */ "",
+			              /* tracePartialFileSuffix = */ "",
+			              InitializeTraceMetrics::True);
+
 			g_network->initTLS();
 			if (!opts.authzPublicKeyFile.empty()) {
 				try {
@@ -2089,7 +2097,6 @@ int main(int argc, char* argv[]) {
 			                              opts.fileSystemPath);
 			g_network->initMetrics();
 			FlowTransport::transport().initMetrics();
-			initTraceEventMetrics();
 		}
 
 		double start = timer(), startNow = now();

flow/Trace.cpp

@@ -170,6 +170,7 @@ public:
 	bool logTraceEventMetrics;
 
 	void initMetrics() {
+		ASSERT(!isOpen());
 		SevErrorNames.init("TraceEvents.SevError"_sr);
 		SevWarnAlwaysNames.init("TraceEvents.SevWarnAlways"_sr);
 		SevWarnNames.init("TraceEvents.SevWarn"_sr);
@@ -430,9 +431,8 @@ public:
 		}
 	}
 
-	void log(int severity, const char* name, UID id, uint64_t event_ts) {
-		if (!logTraceEventMetrics)
-			return;
+	void logMetrics(int severity, const char* name, UID id, uint64_t event_ts) {
+		ASSERT(TraceEvent::isNetworkThread() && logTraceEventMetrics);
 
 		EventMetricHandle<TraceEventNameID>* m = nullptr;
 		switch (severity) {
@@ -781,7 +781,8 @@ void openTraceFile(const Optional<NetworkAddress>& na,
                    std::string baseOfBase,
                    std::string logGroup,
                    std::string identifier,
-                   std::string tracePartialFileSuffix) {
+                   std::string tracePartialFileSuffix,
+                   InitializeTraceMetrics initializeTraceMetrics) {
 	if (g_traceLog.isOpen())
 		return;
 
@@ -808,6 +809,10 @@ void openTraceFile(const Optional<NetworkAddress>& na,
 		baseName = format("%s.0.0.0.0.%d", baseOfBase.c_str(), ::getpid());
 	}
 
+	if (initializeTraceMetrics) {
+		g_traceLog.initMetrics();
+	}
+
 	g_traceLog.open(directory,
 	                baseName,
 	                logGroup,
@@ -821,10 +826,6 @@ void openTraceFile(const Optional<NetworkAddress>& na,
 	g_traceBatch.dump();
 }
 
-void initTraceEventMetrics() {
-	g_traceLog.initMetrics();
-}
-
 void closeTraceFile() {
 	g_traceLog.close();
 }
@@ -1340,17 +1341,17 @@ void BaseTraceEvent::log() {
 
 				if (g_traceLog.isOpen()) {
 					// Log Metrics
-					if (g_traceLog.logTraceEventMetrics && isNetworkThread()) {
+					if (isNetworkThread() && g_traceLog.logTraceEventMetrics) {
 						// Get the persistent Event Metric representing this trace event and push the fields (details)
-						// accumulated in *this to it and then log() it. Note that if the event metric is disabled it
-						// won't actually be logged BUT any new fields added to it will be registered. If the event IS
-						// logged, a timestamp will be returned, if not then 0.  Either way, pass it through to be used
-						// if possible in the Sev* event metrics.
+						// accumulated in *this to it and then logMetrics() it. Note that if the event metric is
+						// disabled it won't actually be logged BUT any new fields added to it will be registered. If
+						// the event IS logged, a timestamp will be returned, if not then 0.  Either way, pass it
+						// through to be used if possible in the Sev* event metrics.
 
 						uint64_t event_ts =
 						    DynamicEventMetric::getOrCreateInstance(format("TraceEvent.%s", type), StringRef(), true)
 						        ->setFieldsAndLogFrom(tmpEventMetric.get());
-						g_traceLog.log(severity, type, id, event_ts);
+						g_traceLog.logMetrics(severity, type, id, event_ts);
 					}
 				}
 			}

flow/include/flow/BooleanParam.h

@@ -20,8 +20,6 @@
 
 #pragma once
 
-#include "flow/Trace.h"
-
 class BooleanParam {
 	bool value;
 
@@ -31,11 +29,6 @@ public:
 	constexpr void set(bool value) { this->value = value; }
 };
 
-template <class BooleanParamSub>
-struct Traceable<BooleanParamSub, std::enable_if_t<std::is_base_of_v<BooleanParam, BooleanParamSub>>> : std::true_type {
-	static std::string toString(BooleanParamSub const& value) { return Traceable<bool>::toString(value); }
-};
-
 // Declares a boolean parametr with the desired name. This declaration can be nested inside of a namespace or another
 // class. This macro should not be used directly unless this boolean parameter is going to be defined as a nested class.
 #define FDB_DECLARE_BOOLEAN_PARAM(ParamName)                                                                           \

flow/include/flow/CodeProbe.h

@@ -281,7 +281,7 @@ struct CodeProbeImpl : ICodeProbe {
 private:
 	CodeProbeImpl() { registerProbe(*this); }
 	inline static CodeProbeImpl _instance;
-	unsigned _hitCount = 0;
+	std::atomic<unsigned> _hitCount = 0;
 	Annotations annotations;
 };
 

flow/include/flow/Trace.h

@@ -31,6 +31,7 @@
 #include <map>
 #include <set>
 #include <type_traits>
+#include "flow/BooleanParam.h"
 #include "flow/IRandom.h"
 #include "flow/Error.h"
 #include "flow/ITrace.h"
@@ -39,6 +40,8 @@
 #define TRACE_DEFAULT_ROLL_SIZE (10 << 20)
 #define TRACE_DEFAULT_MAX_LOGS_SIZE (10 * TRACE_DEFAULT_ROLL_SIZE)
 
+FDB_BOOLEAN_PARAM(InitializeTraceMetrics);
+
 inline int fastrand() {
 	static int g_seed = 0;
 	g_seed = 214013 * g_seed + 2531011;
@@ -539,8 +542,8 @@ void openTraceFile(const Optional<NetworkAddress>& na,
                    std::string baseOfBase = "trace",
                    std::string logGroup = "default",
                    std::string identifier = "",
-                   std::string tracePartialFileSuffix = "");
-void initTraceEventMetrics();
+                   std::string tracePartialFileSuffix = "",
+                   InitializeTraceMetrics initializeTraceMetrics = InitializeTraceMetrics::False);
 void closeTraceFile();
 bool traceFileIsOpen();
 void flushTraceFileVoid();

flow/include/flow/Traceable.h

@@ -29,6 +29,8 @@
 #include <type_traits>
 #include <fmt/format.h>
 
+#include "flow/BooleanParam.h"
+
 #define PRINTABLE_COMPRESS_NULLS 0
 
 template <class IntType>
@@ -245,6 +247,11 @@ struct Traceable<std::atomic<T>> : std::true_type {
 	static std::string toString(const std::atomic<T>& value) { return Traceable<T>::toString(value.load()); }
 };
 
+template <class BooleanParamSub>
+struct Traceable<BooleanParamSub, std::enable_if_t<std::is_base_of_v<BooleanParam, BooleanParamSub>>> : std::true_type {
+	static std::string toString(BooleanParamSub const& value) { return Traceable<bool>::toString(value); }
+};
+
 // Adapter to redirect fmt::formatter calls to Traceable for a supported type
 template <typename T>
 struct FormatUsingTraceable : fmt::formatter<std::string> {