feat(metrics): Use default resource labels for metrics.#14
Conversation
Previously, metrics were reported without the resource labels set via `defaultResource().set()`, using a set of built-in defaults (`service.name`, `service.instance.id`, and `host.name`) instead. This changes the behavior if default resource labels are set and uses those resource labels instead. If no default resource labels are set, the previous behavior is retained for (limited) backwards compatibility. Note on implementation: The `static` keyword was removed from `defaultResource()` to enable correct linking. With `static`, each compilation unit allocates its own set of resource labels, which is not intended.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/OtelMetrics.cpp (1)
22-33: Logic is correct; consider using theempty()accessor.The new early-return path correctly prioritises user-configured resource attributes over the built-in fallbacks. One minor encapsulation improvement:
OTelResourceConfigalready exposesbool empty() const(OtelDefaults.h line 103), so prefer that over direct member access.♻️ Suggested change
static void addCommonResource(JsonObject& resource) { auto &res = OTel::defaultResource(); - if (!res.attrs.empty()) { + if (!res.empty()) { res.addResourceAttributes(resource); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/OtelMetrics.cpp` around lines 22 - 33, The function addCommonResource currently checks res.attrs.empty() on the OTel::defaultResource() return value; change this to use the public accessor by calling res.empty() instead (i.e., use OTelResourceConfig::empty()) so the implementation relies on the type's encapsulated API; update the conditional in addCommonResource that now reads if (!res.attrs.empty()) to if (!res.empty()) and leave the rest of the early-return logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/OtelMetrics.cpp`:
- Around line 22-33: The function addCommonResource currently checks
res.attrs.empty() on the OTel::defaultResource() return value; change this to
use the public accessor by calling res.empty() instead (i.e., use
OTelResourceConfig::empty()) so the implementation relies on the type's
encapsulated API; update the conditional in addCommonResource that now reads if
(!res.attrs.empty()) to if (!res.empty()) and leave the rest of the early-return
logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1eceec2-3476-478b-ae22-9ce983e73955
📒 Files selected for processing (2)
include/OtelDefaults.hsrc/OtelMetrics.cpp
…e merging (#17) * feat(tracer): add SpanKind/StatusCode API and unify resource attribute merging 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 <noreply@anthropic.com> * fix+test: address Copilot review findings on PR #17 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 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Previously, metrics were reported without the resource labels set via
defaultResource().set(), using a set of built-in defaults (service.name,service.instance.id, andhost.name) instead.This changes the behavior if default resource labels are set and uses those resource labels instead. If no default resource labels are set, the previous behavior is retained for (limited) backwards compatibility.
Note on implementation: The
statickeyword was removed fromdefaultResource()to enable correct linking. Withstatic, each compilation unit allocates its own set of resource labels, which is not intended.Summary by CodeRabbit