OTLP/trace improvements (#717)

This PR brings over some changes made to avoid performing setup for otel instrumentation if we are not sending otel information anywhere anyway.

It also adds the ability to configure an OTLP endpoint on the command line using `--otlp-endpoint=<URI>`.

Bear in mind that OTLP support is still not officially supported so this should not be used in production at this stage.

6 files changed, 152 insertions, 35 deletions

diff --git a/src/zenhttp/servers/httpsys.cpp b/src/zenhttp/servers/httpsys.cpp
index c555a39b6..54cc0c22d 100644
--- a/src/zenhttp/servers/httpsys.cpp
+++ b/src/zenhttp/servers/httpsys.cpp
@@ -1647,9 +1647,9 @@ HttpSysTransaction::InvokeRequestHandler(HttpService& Service, IoBuffer Payload)
 	std::string_view Verb = ToString(ThisRequest.RequestVerb());
 	std::string_view Uri  = ThisRequest.m_UriUtf8.ToView();
 
-	ExtendableStringBuilder<64> SpanName;
-	SpanName << Verb << " " << Uri;
-	otel::ScopedSpan HttpSpan(SpanName.ToView(), [&](otel::Span& Span) {
+	auto SpanNamer = [&](StringBuilderBase& SpanName) { SpanName << Verb << " " << Uri; };
+
+	auto SpanAnnotator = [&](otel::Span& Span) {
 		Span.AddAttribute("http.request.method"sv, Verb);
 		Span.AddAttribute("url.path"sv, Uri);
 		// FIXME: should be total size including headers etc according to spec
@@ -1661,7 +1661,9 @@ HttpSysTransaction::InvokeRequestHandler(HttpService& Service, IoBuffer Payload)
 		ExtendableStringBuilder<64> ClientAddr;
 		GetAddressString(ClientAddr, SockAddr, /* IncludePort */ false);
 		Span.AddAttribute("client.address"sv, ClientAddr.ToView());
-	});
+	};
+
+	otel::ScopedSpan HttpSpan(SpanNamer, SpanAnnotator);
 #	endif
 
 	if (!HandlePackageOffers(Service, ThisRequest, m_PackageHandler))
diff --git a/src/zenserver/config/config.cpp b/src/zenserver/config/config.cpp
index 18187711b..07913e891 100644
--- a/src/zenserver/config/config.cpp
+++ b/src/zenserver/config/config.cpp
@@ -132,12 +132,14 @@ ZenServerConfiguratorBase::AddCommonConfigOptions(LuaConfig::Options& LuaOptions
 	LuaOptions.AddOption("server.datadir"sv, ServerOptions.DataDir, "data-dir"sv);
 	LuaOptions.AddOption("server.contentdir"sv, ServerOptions.ContentDir, "content-dir"sv);
 	LuaOptions.AddOption("server.abslog"sv, ServerOptions.AbsLogFile, "abslog"sv);
+	LuaOptions.AddOption("server.otlpendpoint"sv, ServerOptions.OtelEndpointUri, "otlp-endpoint"sv);
 	LuaOptions.AddOption("server.debug"sv, ServerOptions.IsDebug, "debug"sv);
 	LuaOptions.AddOption("server.clean"sv, ServerOptions.IsCleanStart, "clean"sv);
 	LuaOptions.AddOption("server.quiet"sv, ServerOptions.QuietConsole, "quiet"sv);
 	LuaOptions.AddOption("server.noconsole"sv, ServerOptions.NoConsoleOutput, "noconsole"sv);
 
 	////// network
+
 	LuaOptions.AddOption("network.httpserverclass"sv, ServerOptions.HttpConfig.ServerClass, "http"sv);
 	LuaOptions.AddOption("network.httpserverthreads"sv, ServerOptions.HttpConfig.ThreadCount, "http-threads"sv);
 	LuaOptions.AddOption("network.port"sv, ServerOptions.BasePort, "port"sv);
@@ -249,17 +251,18 @@ ZenServerCmdLineOptions::AddCliOptions(cxxopts::Options& options, ZenServerConfi
 
 	// clang-format off
 	options.add_options("logging")
-		("abslog",			"Path to log file",									cxxopts::value<std::string>(AbsLogFile))
-		("log-id",			"Specify id for adding context to log output",		cxxopts::value<std::string>(ServerOptions.LogId))
-		("quiet",			"Configure console logger output to level WARN",	cxxopts::value<bool>(ServerOptions.QuietConsole)->default_value("false"))
-		("noconsole",		"Disable console logging",							cxxopts::value<bool>(ServerOptions.NoConsoleOutput)->default_value("false"))
-		("log-trace", 		"Change selected loggers to level TRACE", 			cxxopts::value<std::string>(ServerOptions.Loggers[logging::level::Trace]))
-		("log-debug", 		"Change selected loggers to level DEBUG", 			cxxopts::value<std::string>(ServerOptions.Loggers[logging::level::Debug]))
-		("log-info", 		"Change selected loggers to level INFO", 			cxxopts::value<std::string>(ServerOptions.Loggers[logging::level::Info]))
-		("log-warn", 		"Change selected loggers to level WARN", 			cxxopts::value<std::string>(ServerOptions.Loggers[logging::level::Warn]))
-		("log-error", 		"Change selected loggers to level ERROR", 			cxxopts::value<std::string>(ServerOptions.Loggers[logging::level::Err]))
-		("log-critical", 	"Change selected loggers to level CRITICAL", 		cxxopts::value<std::string>(ServerOptions.Loggers[logging::level::Critical]))
-		("log-off", 		"Change selected loggers to level OFF", 			cxxopts::value<std::string>(ServerOptions.Loggers[logging::level::Off]))
+		("abslog",			"Path to log file",											cxxopts::value<std::string>(AbsLogFile))
+		("log-id",			"Specify id for adding context to log output",				cxxopts::value<std::string>(ServerOptions.LogId))
+		("quiet",			"Configure console logger output to level WARN",			cxxopts::value<bool>(ServerOptions.QuietConsole)->default_value("false"))
+		("noconsole",		"Disable console logging",									cxxopts::value<bool>(ServerOptions.NoConsoleOutput)->default_value("false"))
+		("log-trace", 		"Change selected loggers to level TRACE", 					cxxopts::value<std::string>(ServerOptions.Loggers[logging::level::Trace]))
+		("log-debug", 		"Change selected loggers to level DEBUG", 					cxxopts::value<std::string>(ServerOptions.Loggers[logging::level::Debug]))
+		("log-info", 		"Change selected loggers to level INFO", 					cxxopts::value<std::string>(ServerOptions.Loggers[logging::level::Info]))
+		("log-warn", 		"Change selected loggers to level WARN", 					cxxopts::value<std::string>(ServerOptions.Loggers[logging::level::Warn]))
+		("log-error", 		"Change selected loggers to level ERROR", 					cxxopts::value<std::string>(ServerOptions.Loggers[logging::level::Err]))
+		("log-critical", 	"Change selected loggers to level CRITICAL", 				cxxopts::value<std::string>(ServerOptions.Loggers[logging::level::Critical]))
+		("log-off", 		"Change selected loggers to level OFF", 					cxxopts::value<std::string>(ServerOptions.Loggers[logging::level::Off]))
+		("otlp-endpoint", 	"OpenTelemetry endpoint URI (e.g http://localhost:4318)", 	cxxopts::value<std::string>(ServerOptions.OtelEndpointUri))
 	;
 	// clang-format on
 
diff --git a/src/zenserver/config/config.h b/src/zenserver/config/config.h
index 40639da13..7c3192a1f 100644
--- a/src/zenserver/config/config.h
+++ b/src/zenserver/config/config.h
@@ -64,6 +64,8 @@ struct ZenServerConfig
 	std::string			  ChildId;			// Id assigned by parent process (used for lifetime management)
 	std::string			  LogId;			// Id for tagging log output
 	std::string			  Loggers[zen::logging::level::LogLevelCount];
+	std::string			  OtelEndpointUri;	// OpenTelemetry endpoint URI
+
 #if ZEN_WITH_TRACE
 	bool		 HasTraceCommandlineOptions = false;
 	TraceOptions TraceCmdLineOptions;
diff --git a/src/zenserver/diag/logging.cpp b/src/zenserver/diag/logging.cpp
index 80da240e8..4962b9006 100644
--- a/src/zenserver/diag/logging.cpp
+++ b/src/zenserver/diag/logging.cpp
@@ -78,12 +78,11 @@ InitializeServerLogging(const ZenServerConfig& InOptions, bool WithCacheService)
 		spdlog::register_logger(ZenClientLogger);
 	}
 
-	//
-
 #if ZEN_WITH_OTEL
-	if (false)
+	if (!InOptions.OtelEndpointUri.empty())
 	{
-		auto OtelSink = std::make_shared<zen::logging::OtelHttpProtobufSink>("http://signoz.localdomain:4318");
+		// TODO: Should sanity check that endpoint is reachable? Also, a valid URI?
+		auto OtelSink = std::make_shared<zen::logging::OtelHttpProtobufSink>(InOptions.OtelEndpointUri);
 		zen::logging::Default().SpdLogger->sinks().push_back(std::move(OtelSink));
 	}
 #endif
diff --git a/src/zentelemetry/include/zentelemetry/otlptrace.h b/src/zentelemetry/include/zentelemetry/otlptrace.h
index f191241ed..49dd90358 100644
--- a/src/zentelemetry/include/zentelemetry/otlptrace.h
+++ b/src/zentelemetry/include/zentelemetry/otlptrace.h
@@ -4,6 +4,7 @@
 
 #include <zenbase/refcount.h>
 #include <zencore/memcmp.h>
+#include <zencore/string.h>
 #include <zencore/uid.h>
 
 #include <span>
@@ -27,12 +28,26 @@ class MemoryArena;
 
 namespace zen::otel {
 
-using AttributeList = std::span<std::pair<std::string, std::string>>;
+using AttributeList = std::span<std::pair<std::string_view, std::string_view>>;
 
 class Tracer;
 
-// OLTP Span ID
+/** Check if OTLP tracing is enabled
+ *
+ * This can be used to avoid unnecessary work when tracing is disabled.
+ *
+ * In many cases it is preferable and more convenient to use the ScopedSpan
+ * constructor  which takes a naming function to avoid string formatting work
+ * when tracing is disabled.
+ */
+bool IsOtlpTraceEnabled();
 
+/** OTLP Span ID
+ *
+ * A SpanId is an 8-byte identifier for a span within a trace. It's not something
+ * that should be interpreted or manipulated directly, but it can be used
+ * for correlating spans during analysis.
+ */
 struct SpanId
 {
 	constexpr static size_t kSize = 8;
@@ -60,7 +75,12 @@ private:
 	uint8_t Id[kSize];
 };
 
-// OLTP Trace ID
+/** OTLP Trace ID
+ *
+ * A TraceId is a 16-byte identifier for a trace. It's not something
+ * that should be interpreted or manipulated directly, but it can be used
+ * for correlating traces during analysis.
+ */
 
 struct TraceId
 {
@@ -68,23 +88,35 @@ struct TraceId
 
 	std::span<const uint8_t> GetBytes() const { return std::span<const uint8_t>(Id, kSize); }
 
-	inline TraceId() noexcept : Id{0} {}
+	inline TraceId() noexcept { memset(Id, 0, kSize); }
 	explicit TraceId(const std::span<const uint8_t, kSize> Bytes) noexcept { std::copy(Bytes.begin(), Bytes.end(), Id); }
 
 	std::strong_ordering operator<=>(const TraceId& Rhs) const noexcept
 	{
-		int cmp = MemCmpFixed<kSize>(Id, Rhs.Id);
-		if (cmp < 0)
+		if (int Diff = MemCmpFixed<kSize>(Id, Rhs.Id); Diff < 0)
+		{
 			return std::strong_ordering::less;
-		if (cmp > 0)
+		}
+		else if (Diff > 0)
+		{
 			return std::strong_ordering::greater;
-		return std::strong_ordering::equal;
+		}
+		else
+		{
+			return std::strong_ordering::equal;
+		}
 	}
+
 	inline operator bool() const
 	{
 		return !(reinterpret_cast<const uint64_t*>(Id)[0] == 0 && reinterpret_cast<const uint64_t*>(Id)[1] == 0);
 	}
 
+	// Generates a new TraceId. The current scheme uses the session ID
+	// as the first 12 bytes, and a counter for the last 4 bytes. This makes
+	// it more likely that traces from the same session can be correlated, and
+	// should make indexes more efficient since the identifiers will be emitted
+	// in a mostly (lexically) increasing order.
 	static TraceId NewTraceId();
 
 	inline const char* GetData() const { return reinterpret_cast<const char*>(Id); }
@@ -93,6 +125,17 @@ private:
 	uint8_t Id[kSize];
 };
 
+/** OTEL attribute key-value pair
+ *
+ * An AttributePair is a key-value pair that provides additional information
+ * about a span or event. This class is intended to support a variety of
+ * value types, but currently only supports string and integer values.
+ *
+ * This is an internal structure used by the Span class, which encodes
+ * the key-value pair efficiently. Each instance is allocated within
+ * a MemoryArena associated with the trace and will be freed without
+ * explicit deallocation when the arena is destroyed.
+ */
 struct AttributePair
 {
 	const char* Key = nullptr;
@@ -129,6 +172,12 @@ struct AttributePair
 	AttributePair* Next = nullptr;
 };
 
+/** OTEL event
+ *
+ * An event represents a time-stamped annotation of the span, consisting
+ * of a name and optional attributes.
+ *
+ */
 struct Event
 {
 	Event*		   NextEvent  = nullptr;
@@ -142,6 +191,10 @@ struct Event
  * A span represents a single operation within a trace. Spans can be nested
  * to form a trace tree.
  *
+ * This class is reference counted in order to support spans which may
+ * cross thread boundaries. A single span may be referenced by multiple threads
+ * and will be finalized when the last reference is released.
+ *
  */
 
 struct Span final : public TRefCounted<Span>
@@ -225,7 +278,21 @@ private:
 class ScopedSpan final
 {
 public:
+	/** Create a new scoped span
+	 *
+	 * @param Name Name of the span
+	 */
 	ScopedSpan(std::string_view Name);
+
+	/** Create a new scoped span with an initializer function
+	 *
+	 * This allows initializing the span (e.g. adding attributes etc) in a safe way
+	 * that avoids unnecessary work when OTLP tracing is disabled.
+	 *
+	 * @param Name Name of the span
+	 * @param InitializerFunction Function which is called with the newly created span
+	 *        so that it can be initialized (e.g. adding attributes etc)
+	 */
 	ScopedSpan(std::string_view Name, std::invocable<Span&> auto&& InitializerFunction) : ScopedSpan(Name)
 	{
 		if (m_Span)
@@ -233,6 +300,43 @@ public:
 			InitializerFunction(*m_Span);
 		}
 	}
+
+	/** Construct a new span with a naming function
+	 *
+	 * The naming function will only be called if OTLP tracing is enabled. This can be
+	 * used to avoid unnecessary string formatting when tracing is disabled.
+	 *
+	 * @param NamingFunction Function which is called with a string builder to create the span name
+	 */
+	ScopedSpan(std::invocable<StringBuilderBase&> auto&& NamingFunction)
+	{
+		if (!IsOtlpTraceEnabled())
+		{
+			return;
+		}
+
+		ExtendableStringBuilder<128> NameBuilder;
+		NamingFunction(NameBuilder);
+	}
+
+	/** Construct a new span with a naming function AND initializer function
+	 *
+	 * Both functions will only be called if OTLP tracing is enabled. This can be
+	 * used to avoid unnecessary string formatting and span initialization related work
+	 * when tracing.
+	 *
+	 * @param NamingFunction Function which is called with a string builder to create the span name
+	 * @param InitializerFunction Function which is called with the newly created span
+	 *        so that it can be initialized (e.g. adding attributes etc)
+	 */
+	ScopedSpan(std::invocable<StringBuilderBase&> auto&& NamingFunction, std::invocable<Span&> auto&& InitializerFunction)
+	: ScopedSpan(NamingFunction)
+	{
+		if (m_Span)
+		{
+			InitializerFunction(*m_Span);
+		}
+	}
 	ScopedSpan() = delete;
 	~ScopedSpan();
 
@@ -241,11 +345,15 @@ public:
 	ScopedSpan(ScopedSpan&& Rhs)				 = default;
 	ScopedSpan(const ScopedSpan& Rhs)			 = default;
 
-		 operator bool() const { return !!m_Span; }
-	void WithSpan(auto Func) const { Func(*m_Span); }
+	// Check if the span is valid (i.e OTLP tracing is enabled)
+	inline explicit operator bool() const { return !!m_Span; }
+
+	// Execute a function with the span pointer if valid. This can
+	// be used to add attributes or events to the span after creation
+	inline void WithSpan(auto Func) const { Func(*m_Span); }
 
 private:
-	ScopedSpan(Span* InSpan, Tracer* InTracer);
+	void Initialize(std::string_view Name);
 
 	Ref<Tracer> m_Tracer;  // This needs to precede the span ref to ensure proper destruction order
 	Ref<Span>	m_Span;
diff --git a/src/zentelemetry/otlptrace.cpp b/src/zentelemetry/otlptrace.cpp
index f987afcfe..6a095cfeb 100644
--- a/src/zentelemetry/otlptrace.cpp
+++ b/src/zentelemetry/otlptrace.cpp
@@ -273,7 +273,7 @@ IsRecording()
 
 std::atomic<bool> g_OtlpTraceEnabled{false};
 
-inline bool
+bool
 IsOtlpTraceEnabled()
 {
 	return g_OtlpTraceEnabled.load();
@@ -346,6 +346,12 @@ ScopedSpan::ScopedSpan(std::string_view Name)
 		return;
 	}
 
+	Initialize(Name);
+}
+
+void
+ScopedSpan::Initialize(std::string_view Name)
+{
 	Tracer* TracerPtr = Tracer::GetTracer();
 
 	Tracer::Impl* const ImplPtr = TracerPtr->m_Impl;
@@ -359,12 +365,9 @@ ScopedSpan::ScopedSpan(std::string_view Name)
 	m_Span	 = NewSpan;
 }
 
-ScopedSpan::ScopedSpan(Span* InSpan, Tracer* InTracer) : m_Tracer(InTracer), m_Span(InSpan)
-{
-}
-
 ScopedSpan::~ScopedSpan()
 {
+	// this is not inline to avoid code bloat on every use site
 }
 
 }  // namespace zen::otel