Add runtime resource attributes, span status/kind, and fix ODR singletons #13
Conversation
…tons Three related improvements: 1. Fix singleton ODR violation across translation units All singleton accessor functions (defaultResource, currentTraceContext, tracerConfig, metricsScopeConfig, defaultMetricLabels, logScopeConfig, defaultLabels) were declared `static inline`, giving each .cpp file its own separate copy of the static local variable. This meant values set in one translation unit (e.g. resource attributes) were invisible to signal builders compiled in other translation units. Changed from `static inline` to `inline` so C++ guarantees a single shared instance across all translation units. 2. Runtime resource attribute support Added buildResourceAttributes() helper that merges runtime defaultResource() values with compile-time fallbacks. All three signal types (traces, metrics, logs) now use this instead of hardcoded addResAttr() calls. This allows callers to set service.name, service.namespace, service.instance.id etc. at runtime via defaultResource().set() and have them appear on all telemetry. 3. Span status and kind API Added setStatus(code, message), setError(message), setOk() for OTLP StatusCode (UNSET=0, OK=1, ERROR=2) and setKind(kind) for SpanKind (INTERNAL=1, SERVER=2, CLIENT=3, PRODUCER=4, CONSUMER=5). Previously kind was hardcoded to SERVER and status was always UNSET, which prevented proper error detection and span classification in backends. Also fixed the move constructor/assignment to transfer the new fields. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
WalkthroughRemoves Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Span as Span
participant Serializer as SpanSerializer
participant Export as Exporter
App->>Span: setStatus(code, "message")
Span->>Span: store statusCode_ and statusMessage_
App->>Span: setKind(kind)
App->>Span: end()
Span->>Serializer: request serialize
Serializer->>Serializer: if statusCode_ != 0
alt status set
Serializer->>Export: emit status { code, message }
end
Serializer->>Export: emit kind_ and other span fields
sequenceDiagram
participant Init as Initialiser
participant Logger as Logger/Metrics/Tracer
participant Helper as buildResourceAttributes()
participant Runtime as RuntimeResource
participant Attrs as AttributesArray
Init->>Logger: prepare resource attributes (fallbacks)
Logger->>Helper: buildResourceAttributes(attrs, svc, inst, host)
Helper->>Runtime: read runtime resource attributes
Helper->>Helper: insert fallback svc/inst/host if missing
Helper->>Attrs: append runtime attributes (overrides allowed)
Logger->>Attrs: use merged attributes for export
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@include/OtelTracer.h`:
- Around line 650-657: The status message is currently serialized whenever
statusCode_ is non-zero, which can emit a message for OK (1); change the
serialization logic in the span status block so the "message" field is only
added when statusCode_ == 2 (ERROR). Specifically, in the code that builds
JsonObject status (using statusCode_ and statusMessage_), guard the assignment
of status["message"] with a check that statusCode_ == 2 and
statusMessage_.length() > 0 so only error statuses include the message.
🧹 Nitpick comments (2)
include/OtelDefaults.h (1)
152-155:static inlineis inconsistent with the rest of the PR's ODR fixes — should be justinline.The entire PR rationale is moving from
static inlinetoinlinefor functions in headers. WhilebuildResourceAttributeshas no static-local state (so there's no singleton bug), marking itstatic inlinestill generates a separate copy in every translation unit that includes this header, needlessly increasing code size on memory-constrained embedded targets.Suggested fix
-static inline void buildResourceAttributes(JsonArray& attrs, +inline void buildResourceAttributes(JsonArray& attrs, const String& fallbackServiceName, const String& fallbackInstanceId, const String& fallbackHostName)include/OtelTracer.h (1)
495-514: Consider usingenum/enum classinstead of rawintfor status codes and span kinds.The magic integers (0/1/2 for status, 1–5 for kind) are easy to misuse. A lightweight
enum classwould provide compile-time safety at zero runtime cost and make call sites self-documenting:span.setKind(OTel::SpanKind::CONSUMER); span.setStatus(OTel::StatusCode::ERROR, "timeout");This would also let you validate inputs or at least constrain the type, rather than accepting any
int.
There was a problem hiding this comment.
Pull request overview
This PR improves OpenTelemetry signal generation for embedded targets by (1) making header-defined “singleton” state truly shared across translation units, (2) enabling runtime-configurable resource attributes across traces/metrics/logs, and (3) adding span kind/status APIs so exported spans can be properly classified and marked as errors.
Changes:
- Fixes ODR/state duplication by switching several
static inlinesingleton accessors toinline(shared across TUs). - Introduces
buildResourceAttributes()and wires it into traces/metrics/logs to merge runtimedefaultResource()attributes with compile-time fallbacks. - Adds span status (
setStatus/setOk/setError) and span kind (setKind) support and serializes them into OTLP JSON.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/OtelMetrics.cpp |
Switches metrics resource attribute emission to use the new merged runtime+fallback resource builder. |
include/OtelTracer.h |
Makes trace context/config singletons TU-shared; adds span kind/status API and serialization. |
include/OtelMetrics.h |
Makes metrics scope config and default label singleton maps TU-shared. |
include/OtelLogger.h |
Makes log scope config and default label singleton maps TU-shared; uses merged resource attributes builder. |
include/OtelDefaults.h |
Makes defaultResource() TU-shared and adds buildResourceAttributes() helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
include/OtelTracer.h:656
setStatus(code, message)accepts a message for any status code, butend()only serializesstatus.messagewhencode == 2(ERROR). This makessetStatus(OK, "...")silently drop the message. Either serialize the message whenever it’s non-empty, or clear/ignore the message insetStatus()for non-ERROR codes to keep the API behavior consistent.
switch (at.type) {
case Type::Str: v["stringValue"] = at.s; break;
case Type::Int: v["intValue"] = at.i; break;
case Type::Dbl: v["doubleValue"] = at.d; break;
case Type::Bool: v["boolValue"] = at.b; break;
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@include/OtelTracer.h`:
- Around line 495-508: The current Span::setStatus(int code, const String&
message = "") clamps out-of-range positive codes to ERROR (statusCode_ = 2),
which can falsely surface issues; change the clamp so any invalid code (code < 0
or code > 2) is set to UNSET (statusCode_ = 0) instead, keep assigning
statusMessage_, and add a debug/logging call (using the project's debug logger
or Serial) inside the invalid-code branch to record the original invalid value
for diagnostics; update the branches in setStatus to reflect this new behavior
and ensure statusCode_ and statusMessage_ are still returned via the Span&
result.
- Line 599: Remove the two unused helper functions mentioned: the
namespace-level addResAttr and the private Span::addResAttr, since
buildResourceAttributes already uses serializeKeyValue; locate the functions by
name (addResAttr and the class method Span::addResAttr) and delete their
declarations and definitions, plus any associated forward declarations or
includes that become unused after removal, then run a build to ensure no
references remain and update any comments mentioning these helpers.
🧹 Nitpick comments (1)
include/OtelTracer.h (1)
727-730: Consider named constants or anenum classfor span kind and status code.Raw
intmagic numbers are easy to misuse (e.g., passing a kind where a status is expected). Even lightweight constants would improve self-documentation and enable compiler-assisted checks:// At namespace level: namespace SpanKind { constexpr int INTERNAL=1, SERVER=2, CLIENT=3, PRODUCER=4, CONSUMER=5; } namespace StatusCode { constexpr int UNSET=0, OK=1, ERROR=2; }This keeps the wire format as
intbut makes call sites likespan.setKind(SpanKind::CLIENT)andspan.setStatus(StatusCode::ERROR, "timeout")self-explanatory.
- Add SpanKind and StatusCode named constants to replace magic ints - Validate setStatus/setKind inputs, clamp invalid codes to UNSET - Only serialize status message for ERROR per OTLP spec - Change buildResourceAttributes from static inline to inline - Use pre-constructed static String keys to avoid heap allocations - Remove dead addResAttr helpers (namespace-level and Span private) - Update include comments that referenced removed helpers Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Existing users may call addResAttr() directly. Rather than removing it outright, mark it [[deprecated]] pointing to buildResourceAttributes(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (kind >= SpanKind::INTERNAL && kind <= SpanKind::CONSUMER) { | ||
| kind_ = kind; | ||
| } else { | ||
| Serial.printf("[otel] WARNING: invalid span kind %d, keeping SERVER\n", kind); |
There was a problem hiding this comment.
setKind() prints "keeping SERVER" on invalid input, but the code actually leaves kind_ unchanged (which may be CLIENT/INTERNAL/etc. if previously set). This warning message is misleading; either update the message to reflect that the previous kind is kept, or explicitly reset kind_ to SERVER when invalid values are provided.
| Serial.printf("[otel] WARNING: invalid span kind %d, keeping SERVER\n", kind); | |
| Serial.printf("[otel] WARNING: invalid span kind %d, keeping previous kind (%d)\n", kind, kind_); |
| Span& setStatus(int code, const String& message = "") { | ||
| if (code >= StatusCode::UNSET && code <= StatusCode::ERROR) { | ||
| statusCode_ = code; | ||
| } else { | ||
| statusCode_ = StatusCode::UNSET; | ||
| Serial.printf("[otel] WARNING: invalid status code %d, defaulting to UNSET\n", code); | ||
| } | ||
| statusMessage_ = message; | ||
| return *this; | ||
| } | ||
| Span& setError(const String& message = "") { | ||
| return setStatus(StatusCode::ERROR, message); | ||
| } | ||
| Span& setOk() { | ||
| return setStatus(StatusCode::OK); | ||
| } | ||
|
|
||
| // ---------- Span kind (OTLP SpanKind) -------------------------------------- | ||
| Span& setKind(int kind) { | ||
| if (kind >= SpanKind::INTERNAL && kind <= SpanKind::CONSUMER) { | ||
| kind_ = kind; | ||
| } else { | ||
| Serial.printf("[otel] WARNING: invalid span kind %d, keeping SERVER\n", kind); | ||
| } |
There was a problem hiding this comment.
setStatus() / setKind() unconditionally emit warnings via Serial.printf, which introduces I/O side effects from a header-only library and may be undesirable in production builds (and printf support varies across Arduino cores). Consider guarding these warnings behind a compile-time flag (e.g., #ifdef DEBUG) or routing through an optional debug/log macro so normal builds stay silent.
| const String& fallbackServiceName, | ||
| const String& fallbackInstanceId, | ||
| const String& fallbackHostName) | ||
| { | ||
| // Pre-constructed keys to avoid per-call temporary String heap allocations | ||
| static const String kServiceName("service.name"); | ||
| static const String kServiceInstanceId("service.instance.id"); | ||
| static const String kHostName("host.name"); | ||
|
|
||
| const auto& res = defaultResource(); | ||
|
|
||
| // Add compile-time fallbacks only for keys not set at runtime | ||
| 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); | ||
| } | ||
|
|
||
| // Add all runtime resource attributes (overrides included) | ||
| for (const auto& p : res.attrs) { | ||
| serializeKeyValue(attrs, p.first, p.second); | ||
| } |
There was a problem hiding this comment.
Indentation in buildResourceAttributes() is inconsistent with the surrounding code in this header (most functions use 2-space indentation). Reformatting this block to match the file’s existing style would improve readability and reduce diff noise in future changes.
| const String& fallbackServiceName, | |
| const String& fallbackInstanceId, | |
| const String& fallbackHostName) | |
| { | |
| // Pre-constructed keys to avoid per-call temporary String heap allocations | |
| static const String kServiceName("service.name"); | |
| static const String kServiceInstanceId("service.instance.id"); | |
| static const String kHostName("host.name"); | |
| const auto& res = defaultResource(); | |
| // Add compile-time fallbacks only for keys not set at runtime | |
| 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); | |
| } | |
| // Add all runtime resource attributes (overrides included) | |
| for (const auto& p : res.attrs) { | |
| serializeKeyValue(attrs, p.first, p.second); | |
| } | |
| const String& fallbackServiceName, | |
| const String& fallbackInstanceId, | |
| const String& fallbackHostName) | |
| { | |
| // Pre-constructed keys to avoid per-call temporary String heap allocations | |
| static const String kServiceName("service.name"); | |
| static const String kServiceInstanceId("service.instance.id"); | |
| static const String kHostName("host.name"); | |
| const auto& res = defaultResource(); | |
| // Add compile-time fallbacks only for keys not set at runtime | |
| 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); | |
| } | |
| // Add all runtime resource attributes (overrides included) | |
| for (const auto& p : res.attrs) { | |
| serializeKeyValue(attrs, p.first, p.second); | |
| } |
1 participant
Summary
Fix singleton ODR violation: All singleton accessor functions (
defaultResource,currentTraceContext,tracerConfig, etc.) werestatic inline, giving each translation unit its own copy. Changed toinlineso state is shared across TUs — without this, runtime resource attributes set in one.cppwere invisible to signal builders in others.Runtime resource attributes: Added
buildResourceAttributes()that mergesdefaultResource()values with compile-time fallbacks. All signal types (traces, metrics, logs) now use this, allowing callers to setservice.name,service.namespace,service.instance.idetc. at runtime.Span status and kind API: Added
setStatus(code, msg),setError(msg),setOk()for OTLP StatusCode andsetKind(kind)for SpanKind. Previously kind was hardcoded to SERVER and status was always UNSET, preventing proper error detection and span classification in backends like Dash0/Grafana.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements