From b717c3c28f9293af144e1caf5a49c3d4e7842a58 Mon Sep 17 00:00:00 2001 From: Matthew Macdonald-Wallace Date: Fri, 5 Jun 2026 13:32:40 +0100 Subject: [PATCH 1/2] feat(tracer): add SpanKind/StatusCode API and unify resource attribute merging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three related improvements that were developed on feat/runtime-resource-attrs-and-span-status but never merged, now applied cleanly on top of main (which already has the ODR singleton fix from #16). 1. Span kind and status API Add SpanKind::{INTERNAL,SERVER,CLIENT,PRODUCER,CONSUMER} and StatusCode::{UNSET,OK,ERROR} constant namespaces. Add Span::setKind(), setStatus(), setError(), setOk(). The JSON serialiser now writes kind_ (defaulting to SERVER) and emits a status block only when statusCode_ != UNSET, per the OTLP spec. Move constructor and move-assignment updated to transfer the new fields. 2. buildResourceAttributes() — unified resource attribute merging Add buildResourceAttributes() to OtelDefaults.h. It merges runtime defaultResource() values with compile-time fallbacks (service.name, service.instance.id, host.name): runtime wins for any key that is set, compile-time fallback fills the rest. All three signal paths (traces, metrics, logs) now use this helper instead of three separate addResAttr() call sites. Replaces the all-or-nothing if/else approach introduced in #14 for metrics. 3. addResAttr() deprecated Mark the global addResAttr() helper [[deprecated]] pointing to buildResourceAttributes(). The private Span::addResAttr() copy is unchanged — it is only referenced by its own class. Co-Authored-By: Claude Sonnet 4.6 --- include/OtelDefaults.h | 27 ++++++++++++++ include/OtelLogger.h | 6 +-- include/OtelMetrics.h | 2 +- include/OtelTracer.h | 83 ++++++++++++++++++++++++++++++++++++------ src/OtelMetrics.cpp | 11 +----- 5 files changed, 104 insertions(+), 25 deletions(-) diff --git a/include/OtelDefaults.h b/include/OtelDefaults.h index cea3476..e92479a 100644 --- a/include/OtelDefaults.h +++ b/include/OtelDefaults.h @@ -150,6 +150,33 @@ inline OTelResourceConfig& defaultResource() { return rc; } +/** + * Populate an OTLP resource attributes array by merging runtime defaultResource() + * values with compile-time fallbacks. Runtime values always win: if a key is + * set via defaultResource().set(), the fallback for that key is suppressed. + */ +inline void buildResourceAttributes(JsonArray& attrs, + const String& fallbackServiceName, + const String& fallbackInstanceId, + const String& fallbackHostName) +{ + static const String kServiceName("service.name"); + static const String kServiceInstanceId("service.instance.id"); + static const String kHostName("host.name"); + + const auto& res = defaultResource(); + + if (res.attrs.find(kServiceName) == res.attrs.end()) + serializeKeyValue(attrs, kServiceName, fallbackServiceName); + if (res.attrs.find(kServiceInstanceId) == res.attrs.end()) + serializeKeyValue(attrs, kServiceInstanceId, fallbackInstanceId); + if (res.attrs.find(kHostName) == res.attrs.end()) + serializeKeyValue(attrs, kHostName, fallbackHostName); + + for (const auto& p : res.attrs) + serializeKeyValue(attrs, p.first, p.second); +} + } // namespace OTel #endif // OTEL_DEFAULTS_H diff --git a/include/OtelLogger.h b/include/OtelLogger.h index 464ac03..ac50528 100644 --- a/include/OtelLogger.h +++ b/include/OtelLogger.h @@ -8,7 +8,7 @@ #include #include "OtelDefaults.h" // expects: nowUnixNano() #include "OtelSender.h" // expects: OTelSender::sendJson(path, doc) -#include "OtelTracer.h" // provides: currentTraceContext(), u64ToStr(), defaults & addResAttr helpers +#include "OtelTracer.h" // provides: currentTraceContext(), u64ToStr(), buildResourceAttributes(), defaults namespace OTel { @@ -134,9 +134,7 @@ class Logger { // Resource (with attributes to ensure service.name lands) JsonObject resource = rl["resource"].to(); JsonArray rattrs = resource["attributes"].to(); - addResAttr(rattrs, "service.name", defaultServiceName()); - addResAttr(rattrs, "service.instance.id", defaultServiceInstanceId()); - addResAttr(rattrs, "host.name", defaultHostName()); + buildResourceAttributes(rattrs, defaultServiceName(), defaultServiceInstanceId(), defaultHostName()); // Scope JsonObject sl = rl["scopeLogs"].to().add(); diff --git a/include/OtelMetrics.h b/include/OtelMetrics.h index 8ee87e6..b6841e9 100644 --- a/include/OtelMetrics.h +++ b/include/OtelMetrics.h @@ -8,7 +8,7 @@ #include #include "OtelDefaults.h" // expects: nowUnixNano() #include "OtelSender.h" // expects: OTelSender::sendJson(path, doc) -#include "OtelTracer.h" // reuses: u64ToStr(), defaultServiceName(), defaultServiceInstanceId(), defaultHostName(), addResAttr() +#include "OtelTracer.h" // reuses: u64ToStr(), defaultServiceName(), defaultServiceInstanceId(), defaultHostName() namespace OTel { diff --git a/include/OtelTracer.h b/include/OtelTracer.h index 38fee5a..3d63650 100644 --- a/include/OtelTracer.h +++ b/include/OtelTracer.h @@ -473,13 +473,30 @@ static inline String generateSpanId() { -/** Append a string-valued OTLP KeyValue object to a resource attributes array. */ +/** @deprecated Use buildResourceAttributes() instead. */ +[[deprecated("Use buildResourceAttributes() instead")]] static inline void addResAttr(JsonArray& arr, const char* key, const String& value) { JsonObject a = arr.add(); a["key"] = key; a["value"].to()["stringValue"] = value; } +/** OTLP SpanKind enum values. */ +namespace SpanKind { + constexpr int INTERNAL = 1; + constexpr int SERVER = 2; + constexpr int CLIENT = 3; + constexpr int PRODUCER = 4; + constexpr int CONSUMER = 5; +} + +/** OTLP StatusCode enum values. */ +namespace StatusCode { + constexpr int UNSET = 0; + constexpr int OK = 1; + constexpr int ERROR = 2; +} + /** Instrumentation scope name and version emitted on every trace payload. */ struct TracerConfig { String scopeName{"otel-embedded"}; @@ -535,6 +552,9 @@ class Span { prevSpanId_(std::move(o.prevSpanId_)), attrs_(std::move(o.attrs_)), events_(std::move(o.events_)), + kind_(o.kind_), + statusCode_(o.statusCode_), + statusMessage_(std::move(o.statusMessage_)), ended_(o.ended_) { o.ended_ = true; // source dtor becomes a no-op @@ -551,16 +571,49 @@ class Span { startNs_ = o.startNs_; prevTraceId_ = std::move(o.prevTraceId_); prevSpanId_ = std::move(o.prevSpanId_); - attrs_ = std::move(o.attrs_); - events_ = std::move(o.events_); - ended_ = o.ended_; - o.ended_ = true; // source won't end() again + attrs_ = std::move(o.attrs_); + events_ = std::move(o.events_); + kind_ = o.kind_; + statusCode_ = o.statusCode_; + statusMessage_ = std::move(o.statusMessage_); + ended_ = o.ended_; + o.ended_ = true; // source won't end() again o.prevTraceId_ = ""; o.prevSpanId_ = ""; } return *this; } + /** Set the OTLP SpanKind. Use the SpanKind:: constants. */ + Span& setKind(int kind) { + if (kind >= SpanKind::INTERNAL && kind <= SpanKind::CONSUMER) { + kind_ = kind; + } else { + DBG_PRINT("[otel] WARNING: invalid span kind "); DBG_PRINT(kind); + DBG_PRINT(", keeping current ("); DBG_PRINT(kind_); DBG_PRINTLN(")"); + } + return *this; + } + + /** Set span status explicitly. Use the StatusCode:: constants. */ + Span& setStatus(int code, const String& message = "") { + if (code >= StatusCode::UNSET && code <= StatusCode::ERROR) { + statusCode_ = code; + statusMessage_ = message; + } else { + DBG_PRINT("[otel] WARNING: invalid status code "); DBG_PRINT(code); + DBG_PRINTLN(", defaulting to UNSET"); + statusCode_ = StatusCode::UNSET; + } + return *this; + } + + /** Shorthand for setStatus(StatusCode::ERROR, message). */ + Span& setError(const String& message = "") { return setStatus(StatusCode::ERROR, message); } + + /** Shorthand for setStatus(StatusCode::OK). */ + Span& setOk() { return setStatus(StatusCode::OK); } + /** @{ Add a typed attribute to the span. Attributes are buffered until @c end(). */ Span& setAttribute(const String& key, const String& v) { //attrs_.push_back(Attr{key, Type::Str, v, 0, 0.0, false}); @@ -680,9 +733,7 @@ class Span { // resourceSpans[0].resource.attributes[...] JsonArray rattrs = doc["resourceSpans"][0]["resource"]["attributes"].to(); - addResAttr(rattrs, "service.name", defaultServiceName()); - addResAttr(rattrs, "service.instance.id", defaultServiceInstanceId()); - addResAttr(rattrs, "host.name", defaultHostName()); + buildResourceAttributes(rattrs, defaultServiceName(), defaultServiceInstanceId(), defaultHostName()); // instrumentation scope JsonObject scope = doc["resourceSpans"][0]["scopeSpans"][0]["scope"].to(); @@ -694,7 +745,7 @@ class Span { s["traceId"] = traceId_; s["spanId"] = spanId_; s["name"] = name_; - s["kind"] = 2; // SERVER by default; adjust if you have a setter + s["kind"] = kind_; s["startTimeUnixNano"] = u64ToStr(startNs_); s["endTimeUnixNano"] = u64ToStr(endNs); @@ -744,6 +795,14 @@ class Span { } } + // Span status — only serialise if explicitly set (UNSET is the default) + if (statusCode_ != StatusCode::UNSET) { + JsonObject status = s["status"].to(); + status["code"] = statusCode_; + if (statusCode_ == StatusCode::ERROR && statusMessage_.length() > 0) + status["message"] = statusMessage_; + } + // Send OTelSender::sendJson("/v1/traces", doc); #endif // OTEL_EXPORTER_OTLP_PROTOCOL_HTTP_PROTOBUF @@ -798,11 +857,13 @@ class Span { String prevTraceId_; String prevSpanId_; - // NEW: buffers std::vector attrs_; std::vector events_; - // RAII guard + int kind_ = SpanKind::SERVER; + int statusCode_ = StatusCode::UNSET; + String statusMessage_; + bool ended_ = false; }; diff --git a/src/OtelMetrics.cpp b/src/OtelMetrics.cpp index 3153929..1b93a82 100644 --- a/src/OtelMetrics.cpp +++ b/src/OtelMetrics.cpp @@ -19,17 +19,10 @@ static void addPointAttributes(JsonArray& attrArray, } } -/** Populate the OTLP resource object from defaultResource() or compile-time defaults. */ +/** Populate the OTLP resource object, merging runtime and compile-time defaults. */ static void addCommonResource(JsonObject& resource) { - auto &res = OTel::defaultResource(); - if (!res.empty()) { - res.addResourceAttributes(resource); - return; - } JsonArray rattrs = resource["attributes"].to(); - addResAttr(rattrs, "service.name", defaultServiceName()); - addResAttr(rattrs, "service.instance.id", defaultServiceInstanceId()); - addResAttr(rattrs, "host.name", defaultHostName()); + buildResourceAttributes(rattrs, defaultServiceName(), defaultServiceInstanceId(), defaultHostName()); } /** Write the instrumentation scope name and version into @p scope. */ From c999e86af40a63d89f3be96a3907dcc7467568ff Mon Sep 17 00:00:00 2001 From: Matthew Macdonald-Wallace Date: Fri, 5 Jun 2026 13:45:17 +0100 Subject: [PATCH 2/2] fix+test: address Copilot review findings on PR #17 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Fix Span move-assignment TraceContext corruption If the RHS span was the currently active context, calling end() on the LHS would restore currentTraceContext() to the LHS parent's IDs, leaving subsequent spans/logs linking to the wrong parent. Now the RHS active state is checked before end(), and if it was active the context is reinstalled after the field move. 2. Add tests for span kind and status serialisation - default kind is SERVER (2) - setKind(CLIENT) → kind field is 3 - status block absent when UNSET (default) - setError("msg") → status.code=2 + status.message present - setOk() → status.code=1, no status.message 3. Add tests for buildResourceAttributes() merge behaviour - runtime service.name override appears in span resource attributes - partial runtime override (only service.name set) still yields compile-time fallback for service.instance.id and host.name All 20 native tests pass. Co-Authored-By: Claude Sonnet 4.6 --- include/OtelTracer.h | 26 ++++++--- test/test_otlp/test_otlp.cpp | 110 ++++++++++++++++++++++++++++++++++- 2 files changed, 127 insertions(+), 9 deletions(-) diff --git a/include/OtelTracer.h b/include/OtelTracer.h index 3d63650..ded0ae0 100644 --- a/include/OtelTracer.h +++ b/include/OtelTracer.h @@ -564,22 +564,32 @@ class Span { Span& operator=(Span&& o) noexcept { if (this != &o) { - if (!ended_) end(); // finish our current span if still open - name_ = std::move(o.name_); - traceId_ = std::move(o.traceId_); - spanId_ = std::move(o.spanId_); - startNs_ = o.startNs_; - prevTraceId_ = std::move(o.prevTraceId_); - prevSpanId_ = std::move(o.prevSpanId_); + // Check whether the RHS is the active span BEFORE end() restores the context. + // If it is, end() will clobber the context with the LHS parent's IDs, and + // subsequent spans/logs would link to the wrong parent. + bool rhs_was_active = (currentTraceContext().traceId == o.traceId_ && + currentTraceContext().spanId == o.spanId_); + if (!ended_) end(); + name_ = std::move(o.name_); + traceId_ = std::move(o.traceId_); + spanId_ = std::move(o.spanId_); + startNs_ = o.startNs_; + prevTraceId_ = std::move(o.prevTraceId_); + prevSpanId_ = std::move(o.prevSpanId_); attrs_ = std::move(o.attrs_); events_ = std::move(o.events_); kind_ = o.kind_; statusCode_ = o.statusCode_; statusMessage_ = std::move(o.statusMessage_); ended_ = o.ended_; - o.ended_ = true; // source won't end() again + o.ended_ = true; o.prevTraceId_ = ""; o.prevSpanId_ = ""; + // Reinstall the moved-in span as active if the source was active. + if (rhs_was_active && !ended_) { + currentTraceContext().traceId = traceId_; + currentTraceContext().spanId = spanId_; + } } return *this; } diff --git a/test/test_otlp/test_otlp.cpp b/test/test_otlp/test_otlp.cpp index a0e49ce..27e0574 100644 --- a/test/test_otlp/test_otlp.cpp +++ b/test/test_otlp/test_otlp.cpp @@ -150,6 +150,101 @@ void test_span_has_non_empty_trace_id() { TEST_ASSERT_TRUE(strlen(tid) > 0); } +void test_span_default_kind_is_server() { + auto span = OTel::Tracer::startSpan("my-op"); + span.end(); + JsonDocument doc; + deserializeJson(doc, FakeSender::lastJson); + int kind = doc["resourceSpans"][0]["scopeSpans"][0]["spans"][0]["kind"]; + TEST_ASSERT_EQUAL_INT(OTel::SpanKind::SERVER, kind); +} + +void test_span_setKind_client() { + auto span = OTel::Tracer::startSpan("my-op"); + span.setKind(OTel::SpanKind::CLIENT); + span.end(); + JsonDocument doc; + deserializeJson(doc, FakeSender::lastJson); + int kind = doc["resourceSpans"][0]["scopeSpans"][0]["spans"][0]["kind"]; + TEST_ASSERT_EQUAL_INT(OTel::SpanKind::CLIENT, kind); +} + +void test_span_status_absent_when_unset() { + auto span = OTel::Tracer::startSpan("my-op"); + span.end(); + JsonDocument doc; + deserializeJson(doc, FakeSender::lastJson); + TEST_ASSERT_TRUE( + doc["resourceSpans"][0]["scopeSpans"][0]["spans"][0]["status"].isNull()); +} + +void test_span_setError_emits_status_code_and_message() { + auto span = OTel::Tracer::startSpan("my-op"); + span.setError("something failed"); + span.end(); + JsonDocument doc; + deserializeJson(doc, FakeSender::lastJson); + JsonObject status = + doc["resourceSpans"][0]["scopeSpans"][0]["spans"][0]["status"]; + TEST_ASSERT_EQUAL_INT(OTel::StatusCode::ERROR, (int)status["code"]); + TEST_ASSERT_EQUAL_STRING("something failed", (const char*)status["message"]); +} + +void test_span_setOk_emits_status_without_message() { + auto span = OTel::Tracer::startSpan("my-op"); + span.setOk(); + span.end(); + JsonDocument doc; + deserializeJson(doc, FakeSender::lastJson); + JsonObject status = + doc["resourceSpans"][0]["scopeSpans"][0]["spans"][0]["status"]; + TEST_ASSERT_EQUAL_INT(OTel::StatusCode::OK, (int)status["code"]); + TEST_ASSERT_TRUE(status["message"].isNull()); +} + +// ── Resource attribute merging ──────────────────────────────────────────────── + +void test_resource_runtime_value_appears_in_span() { + // setUp sets service.name = "test-service" via defaultResource() + auto span = OTel::Tracer::startSpan("my-op"); + span.end(); + JsonDocument doc; + deserializeJson(doc, FakeSender::lastJson); + JsonArray attrs = doc["resourceSpans"][0]["resource"]["attributes"]; + bool found = false; + for (JsonObject a : attrs) { + if (strcmp(a["key"], "service.name") == 0) { + found = (strcmp(a["value"]["stringValue"], "test-service") == 0); + break; + } + } + TEST_ASSERT_TRUE(found); +} + +void test_resource_partial_override_keeps_fallback_keys() { + // Clear all runtime attrs, set only service.name — the other keys should + // fall back to compile-time defaults (service.instance.id and host.name). + OTel::defaultResource().clear(); + OTel::defaultResource().set("service.name", "partial-override"); + + auto span = OTel::Tracer::startSpan("my-op"); + span.end(); + JsonDocument doc; + deserializeJson(doc, FakeSender::lastJson); + JsonArray attrs = doc["resourceSpans"][0]["resource"]["attributes"]; + + bool hasCustomName = false; + bool hasInstanceId = false; + for (JsonObject a : attrs) { + if (strcmp(a["key"], "service.name") == 0) + hasCustomName = (strcmp(a["value"]["stringValue"], "partial-override") == 0); + if (strcmp(a["key"], "service.instance.id") == 0) + hasInstanceId = true; + } + TEST_ASSERT_TRUE(hasCustomName); + TEST_ASSERT_TRUE(hasInstanceId); +} + // ── Main ────────────────────────────────────────────────────────────────────── int main() { @@ -169,10 +264,23 @@ int main() { RUN_TEST(test_log_body_string_value); RUN_TEST(test_log_error_severity_text); - // Traces + // Traces — basic RUN_TEST(test_span_sends_to_traces_endpoint); RUN_TEST(test_span_name_in_payload); RUN_TEST(test_span_has_non_empty_trace_id); + // Traces — span kind + RUN_TEST(test_span_default_kind_is_server); + RUN_TEST(test_span_setKind_client); + + // Traces — span status + RUN_TEST(test_span_status_absent_when_unset); + RUN_TEST(test_span_setError_emits_status_code_and_message); + RUN_TEST(test_span_setOk_emits_status_without_message); + + // Resource attribute merging + RUN_TEST(test_resource_runtime_value_appears_in_span); + RUN_TEST(test_resource_partial_override_keeps_fallback_keys); + return UNITY_END(); }