fix(sender): guard setHandshakeTimeout behind ESP32-only check#16
Merged
Conversation
Add OTEL_EXPORTER_OTLP_ENDPOINT as the spec-compliant base URL define,
replacing the non-standard OTEL_COLLECTOR_BASE_URL (kept as a legacy
fallback for backward compatibility).
Add per-signal endpoint overrides (OTEL_EXPORTER_OTLP_{LOGS,TRACES,METRICS}_ENDPOINT)
used verbatim per the exporter spec — no path is appended.
Add global and per-signal header configuration
(OTEL_EXPORTER_OTLP_HEADERS, OTEL_EXPORTER_OTLP_{LOGS,TRACES,METRICS}_HEADERS)
as comma-separated key=value pairs, parsed and cached on first send.
Add automatic HTTPS support: when an endpoint URL starts with https://,
a WiFiClientSecure is used. Certificate validation is skipped by default
(OTEL_TLS_INSECURE=1) which is the practical default for embedded devices.
Priority chain for URL resolution:
1. Per-signal endpoint override (used as-is)
2. OTEL_EXPORTER_OTLP_ENDPOINT (paths appended)
3. OTEL_COLLECTOR_BASE_URL (legacy fallback, paths appended)
The OTLP JSON spec encodes AggregationTemporality as an integer:
AGGREGATION_TEMPORALITY_CUMULATIVE = 1
AGGREGATION_TEMPORALITY_DELTA = 2
Previously the value was serialized as a plain string ("DELTA",
"CUMULATIVE") which is not spec-compliant and causes strict endpoints
such as Datadog direct OTLP to silently ignore the field.
The public API is unchanged — callers still pass "DELTA" or "CUMULATIVE"
to sum(). The mapping to the correct integer happens at serialization time.
basic/main.cpp: fix API calls that referenced non-existent methods (Logger::begin, 4-arg Tracer::begin, OTelGauge). Replace with the correct current API: Tracer::begin(scope, version), Metrics::begin, and Metrics::gauge(). Remove the unused OTEL_COLLECTOR_HOST define. direct_otlp/main.cpp: new example demonstrating direct vendor OTLP ingestion with the full attribute and tagging API across all signals — typed span attributes (string/int/double/bool), span events, per-call and default metric labels, per-call and default log labels. Includes build flag examples for Datadog and Grafana Cloud. basic/README.md, direct_otlp/README.md: concise READMEs explaining what each example does and the minimum build flags required.
validate.yml: replace OTEL_COLLECTOR_HOST/PORT with the standard OTEL_EXPORTER_OTLP_ENDPOINT variable, fix actions/checkout version (v6 does not exist, corrected to v4), add build steps for the new direct_otlp example across all three platforms. README.md: update build flags and .env examples to use OTEL_EXPORTER_OTLP_ENDPOINT; fix quick-start snippet to use the correct current API; expand configuration table with all new exporter, header, and TLS macros; mark OTEL_COLLECTOR_BASE_URL as a legacy fallback. library.json: correct repository URL. .gitignore: ignore .claude and .vscode directories.
DELTA = 1, CUMULATIVE = 2 per the OTLP proto spec. Previous commit had these reversed (CUMULATIVE = 1, DELTA = 2), which would have caused DELTA payloads to be interpreted as CUMULATIVE by spec-compliant collectors.
…P signals Implements OTEL_EXPORTER_OTLP_PROTOCOL compile-time switch so callers set -DOTEL_EXPORTER_OTLP_PROTOCOL=OTEL_EXPORTER_OTLP_PROTOCOL_HTTP_PROTOBUF and the library transparently encodes metrics, logs, and traces as binary protobuf instead of JSON — no API changes required. Key changes: - OtelSender: add sendProto(), per-signal headers, HTTPS auto-detection, three-level URL priority (per-signal > base > legacy), contentType in the SPSC queue item - OtelProtoEncoder: new file implementing nanopb encode helpers and all four signal senders (sendGauge, sendSum, sendLog, sendSpan); trace proto is hand-streamed using raw field numbers since it is absent from the reference generated set - Proto descriptor files copied from reference as .pb.inc to prevent PlatformIO auto-compilation; included under the protocol guard - OtelMetrics, OtelLogger, OtelTracer: #if guards route each signal to the proto encoder or JSON path depending on the protocol flag; default labels are merged at the call site for metrics and passed separately for logs - library.json / platformio.ini: add nanopb/Nanopb@^0.4.91 dependency
…y.json - examples/proto_otlp/main.cpp: demonstrates protobuf encoding mode with the same three-signal (trace/metric/log) pattern as direct_otlp; the only difference from the caller's perspective is the build flag -DOTEL_EXPORTER_OTLP_PROTOCOL=OTEL_EXPORTER_OTLP_PROTOCOL_HTTP_PROTOBUF - library.json: add "examples" array so the PlatformIO registry lists all three examples (basic, direct_otlp, proto_otlp) on the library page - platformio.ini: add esp32dev-proto, rpipicow-proto, esp8266-proto envs that extend their base envs and enable the protobuf flag via `extends` - validate.yml: build the basic example (was missing), and build proto_otlp against the new *-proto environments on all three platforms
PlatformIO cached otel-embedded-cpp@1.0.2 from the registry during pio update. Because library.json also declared version 1.0.2, the LDF used the cached copy (with old API calls) instead of --lib ".". - Bump library version to 1.1.0 so no registry match is possible - Add build.srcFilter to exclude main.cpp from library compilation; src/main.cpp is an integration test, not a reusable library source - Flush the cached package before each CI build run as belt-and-suspenders
platformio ci does not reliably propagate platform/board/framework through the extends mechanism, causing UndefinedEnvPlatformError. Replace all three *-proto environments with explicit full definitions.
The nanopb-generated .pb.inc files used full project-rooted paths (e.g. opentelemetry/proto/common/v1/common.pb.h) that required src/proto/ on the include path. PlatformIO only adds src/ by default, causing fatal include errors during CI. Switch to sibling-relative includes since each .pb.h lives in the same directory as its .pb.inc. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The nanopb-generated .pb.h files referenced sibling proto headers using full project-rooted paths (e.g. opentelemetry/proto/common/v1/...) that require src/proto/ on the include path. Switch to relative paths (../../common/v1/common.pb.h etc.) so the files resolve correctly from their own directory, regardless of which include paths the build system adds. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…r.cpp metricsScopeConfig() and logScopeConfig() are defined in their respective signal headers, but OtelProtoEncoder.cpp didn't include them, causing undeclared-identifier errors in the proto build path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Guard the three JSON-only helpers in OtelMetrics.cpp (addPointAttributes, addCommonResource, addCommonScope) with #if !HTTP_PROTOBUF so they are not compiled when the proto path is active, eliminating -Wunused-function warnings. - Delete cb_kv_cstr and cb_typed_attrs from OtelProtoEncoder.cpp; both were defined but never called (dead code from an earlier draft). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a host-compilable test suite (pio test -e native) that exercises the JSON encoding path of all three signals without real hardware: - test/stubs/Arduino.h — minimal String/Serial/millis shim so library headers compile under host GCC - test/native/fake_sender.cpp — replaces OtelSender (WiFi/HTTP) with a stub that captures the last emitted JSON/proto payload for assertion - test/native/test_otlp.cpp — 13 Unity tests covering gauge name/value/ unit/labels, log severity text+number+body, and span name+traceId - [env:native] in platformio.ini with OtelSender.cpp excluded from the src filter and test/stubs/ on the include path - CI step "Run native unit tests" added before the cross-compile steps The tests will need iteration as the stubs are exercised; they are intentionally conservative (no proto path, no collector service) as a starting point. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…_proto library The OTel proto files (common, resource, metrics, logs) are now sourced from the published PlatformIO library aodtorusan/opentelemetry_proto@^1.9.0 rather than being vendored in src/proto/. Changes: - Delete src/proto/ (8 files: 4 .pb.h + 4 .pb.inc) - OtelProtoEncoder.cpp: remove manual .pb.inc includes (the library's .pb.c files are compiled automatically by PlatformIO's LDF) and update .pb.h include paths from "proto/opentelemetry/..." to "opentelemetry/..." (library src/ is on the include path) - library.json: add aodtorusan/opentelemetry_proto@^1.9.0 as a dependency - platformio.ini already has the library in all env lib_deps (added by user) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add aodtorusan/opentelemetry_proto@^1.9.0 to lib_deps in every env (including non-proto envs; the library is a declared dep in library.json so having it available everywhere is consistent) - Restore section comments removed by pio auto-formatting: native env explanation, rpipicow board note, proto variants header Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PlatformIO's native test runner skipped test/native/ because the directory name clashed with the platform name. Renaming to test/test_otlp/ makes the suite discoverable. Also simplified the pio test CI command to remove redundant --project-dir / --project-conf flags that default to the checked-out root. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
OtelDefaults.h used String without including Arduino.h, so when
test_otlp.cpp included it directly (before OtelMetrics.h brought in
the stub), GCC parsed String as its error-recovery int type — making
attrs appear as int and breaking all String-accepting set() overloads.
Adding #include <Arduino.h> to OtelDefaults.h makes it self-contained
regardless of include order (on embedded targets the header is already
included by the framework, so this is a no-op there).
Also adds String indexOf(const String&) overload to the Arduino stub so
OtelTracer.h's indexOf("01") string-literal call compiles cleanly.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Please use `pio pkg update` instead.
When ARDUINO is defined, ArduinoJson expects the Arduino framework to provide pgm_read_byte, __FlashStringHelper, Print, Printable, and Stream. Our stub only had String/Serial/timing, so ArduinoJson's pgmspace.hpp, FlashString.hpp, PrintWriter.hpp, and related headers failed to compile on native/GCC. Adds the full set of types ArduinoJson needs in Arduino mode: - pgm_read_byte/word/ptr macros (identity reads on native) - __FlashStringHelper struct + correct F() cast - Print / Printable / Stream base classes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ArduinoJson's ArduinoStringWriter calls String::concat() to flush its buffer, and ArduinoStreamReader calls Stream::readBytes() to deserialize from a stream. Both were missing from the host test stub. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add `#include <Arduino.h>` first in test_otlp.cpp so ArduinoJson uses the same inline namespace (V743PB42) as fake_sender.cpp, fixing linker errors from namespace mismatch. - Include OtelMetrics.cpp directly in test_otlp.cpp so static-inline singletons (defaultResource, metricsScopeConfig) are shared with test fixtures; project_src.cpp is now a no-op comment. - Set ARDUINOJSON_DEFAULT_NESTING_LIMIT=15 in native build flags — OTLP JSON nests 11 levels deep, exceeding the default of 10, which caused deserializeJson to silently drop the attributes array contents. - Fix deserializeJson(doc, json) ambiguity in OtelTracer.h (String satisfies both IteratorReader and ArduinoStringReader); use json.c_str() for unambiguous const char* reader. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
feat: OTLP exporter spec alignment, protobuf encoding path, and native unit tests
Add /** */ docstrings to all public classes, structs, and methods across OtelSender.h, OtelDefaults.h, OtelMetrics.h, OtelLogger.h, OtelTracer.h, OtelProtoEncoder.h, and src/OtelMetrics.cpp. Converts existing inline // comments on public API to proper Doxygen /** */ blocks, and adds new docstrings to previously undocumented items (TraceContext, ExtractedContext, KeyValuePairs, Propagators, RemoteParentScope, Span, Tracer, utility functions, and all signal encoder declarations). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…r/Tracer/Gauge names The namespace OTel block declared using aliases for OTelLogger, OTelTracer, OTelGauge etc., but those prefixed names don't exist. The actual classes (Logger, Tracer, Metrics) are already defined inside namespace OTel by their respective headers and need no further aliasing. Also removed getDefaultResource() which constructed OTelResourceConfig with 5 positional args that the struct no longer supports.
- examples/direct_otlp/main.cpp: pass 1.0 per call with DELTA temporality instead of accumulating a running total; the old code sent a monotonically growing value under DELTA, which is a semantic error (DELTA expects the increment since the last report, not the cumulative sum). - README.md / include/OtelTracer.h: align macro name — the README table, platformio.ini, OtelEmbeddedCpp.h, and src/main.cpp all use OTEL_SERVICE_INSTANCE, but OtelTracer.h::defaultServiceInstanceId() checked OTEL_SERVICE_INSTANCE_ID. Fixed OtelTracer.h and README to use the consistent OTEL_SERVICE_INSTANCE name. - include/OtelMetrics.h, OtelLogger.h, OtelTracer.h: remove `static` from singleton inline helpers (metricsScopeConfig, defaultMetricLabels, logScopeConfig, defaultLabels, currentTraceContext, tracerConfig). The `static` keyword gives internal linkage, so each translation unit that includes the header gets its own private copy of the function-local static. Calls to Metrics::begin() or Tracer::begin() in one TU were invisible to the encoder in another TU — silently reverting scope metadata and default labels to defaults. Removing `static` while keeping `inline` gives a single process-wide instance as required by the C++ ODR for inline functions. - include/OtelProtoEncoder.h: include OtelSender.h before the protocol guard so the macros are always defined. Without it, if the header were included standalone both OTEL_EXPORTER_OTLP_PROTOCOL and OTEL_EXPORTER_OTLP_PROTOCOL_HTTP_PROTOBUF would expand to 0, making the guard evaluate to true and silently activating the protobuf path. - src/OtelSender.cpp: replace the byte-by-byte append loop in sendProto() with String(const char*, len) to correctly handle null bytes in protobuf payloads and avoid repeated heap reallocations. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Race in addHeader() vs RP2040 worker: addHeader() pushes to per-signal std::vectors that doPost_() iterates from core 1. A concurrent push_back() could reallocate the vector mid-iteration, which is UB. Freeze the headers (set an atomic flag) at the first send so the worker reads a stable list, and reject later addHeader() calls. Documented the new contract: call addHeader() before the first send. - Documented TLS CA cert path was a no-op: the header promised "OTEL_TLS_INSECURE=0 + OTEL_TLS_CA_CERT enables certificate validation", but the implementation only ever called setInsecure() and never read OTEL_TLS_CA_CERT. Now: when OTEL_TLS_INSECURE=0, call setCACert(); if OTEL_TLS_CA_CERT is undefined in that case, fail at compile time with a clear #error rather than silently constructing a client with no trust anchors. - queueIsHealthy() returned false on synchronous platforms because worker_started_ is only set on RP2040. Header docs say "always true on synchronous platforms"; implementation now matches. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…bles CodeRabbit review 4258823480. - Storing protobuf bodies in an Arduino String and passing them to HTTPClient::POST(String) relies on the platform's HTTPClient using String::length() rather than strlen() internally. ESP32, ESP8266, and Arduino-Pico happen to do the right thing, but the type signature says "text" while the contents are arbitrary bytes including embedded NULs. Switch OTelQueuedItem::payload to std::vector<uint8_t> and use HTTPClient::POST(uint8_t*, size_t) directly so the binary contract is enforced at the type system level. JSON callers serialise to a temporary String and copy the bytes into the vector (JSON is null-byte-free, so the copy is safe and the cost is one allocation per send). Free the queue slot's allocation on dequeue (swap with empty vector) rather than holding it until the next overwrite, so a long idle period doesn't pin RAM proportional to the largest payload ever sent. Updated fake_sender.cpp's enqueue_ stub signature to match. - Add compile-time bounds checks for OTEL_PROTO_BUFFER_SIZE (must be > 0) and OTEL_QUEUE_CAPACITY (must be >= 2 — one slot is unusable in an SPSC ring buffer). Misconfiguration now fails fast with a clear #error. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Allocating a fresh WiFiClientSecure inside doPost_() on every send
created two failure modes on memory-tight ESP32 silicon:
1. The mbedtls TLS context alloc (~30 KB) needs contiguous heap
blocks for BIGNUM/ECP/MD working memory during handshake. Under
heap pressure (BT stack + WiFi/lwIP + tasks resident on the
ESP32-2432S028 silicon) those allocations fail with err=-19840
(ECP) or err=-20864 (MD), and every send drops silently.
2. Even when handshake succeeds, doing a full TLS negotiation per
send blocks the worker for ~500 ms per item and pegs the CPU on
mbedtls EC math.
Move the WiFiClientSecure + HTTPClient to file scope, initialize
once on first send, and reuse the socket via HTTPClient::setReuse(true)
across all subsequent sends. OTLP signals all live under the same
endpoint host so the connection survives across metric/log/trace
boundaries.
Plain HTTP path keeps the per-call allocation pattern — exporters
rarely sit behind plain HTTP in production and the persistent
secure-client state shouldn't leak into an unrelated http session.
setHandshakeTimeout() is only declared in ESP32's NetworkClientSecure.h. It is absent from both the Earle Philhower RP2040 WiFiClientSecure and the ESP8266 BearSSL WiFiClientSecure, causing a compile error on those targets. Wrap the call in #if defined(ESP32); the socket-level setTimeout() that follows is available on all three platforms and acts as a fallback timeout on ESP8266 and RP2040. Verified: esp32dev, rpipicow, and esp8266 all compile cleanly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds OTLP exporter spec-aligned endpoint/header macros, optional nanopb protobuf encoding for all signals, a binary-safe sender with per-signal endpoints/headers, expanded CI (native Unity tests), README and example sketches, and host test stubs plus a FakeSender for payload assertions. ChangesBuild, CI and Manifests
Sender Infrastructure and HTTP/TLS Layer
Protobuf Encoding Infrastructure
Signal-Specific Protocol Paths
Examples and Documentation
Native Host Testing Infrastructure
Sequence DiagramsequenceDiagram
participant App as Example Sketch
participant Metrics as OTel::Metrics
participant Router as Protocol Router
participant Proto as OTel::Proto
participant JSON as ArduinoJson
participant Sender as OTelSender
App->>Metrics: OTel::Metrics::gauge(name,value,unit)
Metrics->>Router: buildAndSendGauge(name,value,unit,labels)
alt OTEL_EXPORTER_OTLP_PROTOCOL_HTTP_PROTOBUF
Router->>Proto: sendGauge(name,value,unit,mergedLabels)
Proto->>Sender: sendProto("/v1/metrics", buf, len)
Sender->>Sender: enqueue_(path,"application/x-protobuf",payload)
else
Router->>JSON: build OTLP JSON MetricsData
JSON->>Sender: sendJson("/v1/metrics", doc)
Sender->>Sender: enqueue_(path,"application/json",payload)
end
Sender->>Sender: worker drains queue and POSTs to endpoint
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands the OTLP sender and overall project setup to improve portability and spec alignment across embedded targets, while also adding an OTLP/HTTP protobuf encoding path and host-based unit tests. It also includes the stated ESP32-only guard for WiFiClientSecure::setHandshakeTimeout() to fix non-ESP32 build failures.
Changes:
- Guard ESP32-only TLS handshake timeout configuration and refactor
OTelSenderto support per-signal endpoints/headers and protobuf payloads. - Add an OTLP/protobuf encoder implementation and wire it into metrics/logs/traces.
- Add native (host) Unity tests plus CI/workflow updates and new/updated examples & documentation.
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/OtelSender.cpp |
Refactors HTTP(S) send path (persistent TLS client), adds endpoint/header parsing, and introduces protobuf send support. |
include/OtelSender.h |
Adds configuration macros (endpoints/headers/TLS/protocol), queue payload as bytes, and sendProto()/addHeader() APIs. |
src/OtelProtoEncoder.cpp |
Implements OTLP/HTTP protobuf encoding using nanopb for metrics/logs and manual encoding for traces. |
include/OtelProtoEncoder.h |
Declares protobuf encoder API behind a protocol-selection compile-time guard. |
src/OtelMetrics.cpp |
Switches metrics encoding depending on JSON vs protobuf and fixes temporality encoding in JSON to spec integer enum. |
include/OtelMetrics.h |
Documentation and minor API/comment improvements for metrics facade and defaults. |
include/OtelLogger.h |
Adds protobuf-path emission and improves documentation for logging API and severity mapping. |
include/OtelTracer.h |
Adds protobuf-path span emission and expands documentation for tracing/context propagation utilities. |
include/OtelDefaults.h |
Ensures Arduino types are available and improves inline docs for defaults/resource config helpers. |
include/OtelEmbeddedCpp.h |
Removes legacy alias block; now acts as a convenience umbrella include + macro defaults. |
test/test_otlp/test_otlp.cpp |
Adds native Unity tests validating emitted OTLP JSON structure/paths for metrics/logs/traces. |
test/test_otlp/fake_sender.h |
Declares fake sender capture state for unit tests. |
test/test_otlp/fake_sender.cpp |
Implements OTelSender stubs for tests to capture JSON/protobuf payloads without hardware. |
test/test_otlp/project_src.cpp |
Notes TU strategy for sharing static-inline singletons with fixtures. |
test/stubs/Arduino.h |
Adds a minimal host Arduino shim to compile library + ArduinoJson under native GCC. |
platformio.ini |
Adds native env for host tests and adds protobuf build variants for CI matrix. |
.github/workflows/validate.yml |
Runs native unit tests and expands CI build coverage across examples and protobuf variants. |
README.md |
Updates configuration docs to spec-aligned endpoint/header macros and refreshes example code. |
library.json |
Bumps version, adds deps, updates repository URL, and declares examples metadata. |
examples/basic/main.cpp |
Updates the basic example to new configuration macros and current API usage. |
examples/basic/README.md |
Documents the basic example and build-flag configuration. |
examples/direct_otlp/main.cpp |
Adds a direct-to-vendor OTLP example demonstrating headers/endpoints and signal APIs. |
examples/direct_otlp/README.md |
Documents the direct OTLP example and common vendor configs. |
examples/proto_otlp/main.cpp |
Adds protobuf-mode example demonstrating identical app code with protocol flag change. |
.gitignore |
Ignores local dev artifacts (.claude, .vscode). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
.github/workflows/validate.yml (2)
15-15: ⚡ Quick winConsider pinning GitHub Actions to commit SHAs.
GitHub Actions are currently pinned to tags (
@v6), which are mutable references. For supply-chain security, pin to immutable commit SHAs with a comment indicating the version, e.g.:- uses: actions/checkout@a81bbbf8298c0fa03ea29cdc473d45769f953675 # v6Also applies to: 18-18
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/validate.yml at line 15, Replace mutable action tags with immutable commit SHAs: find the occurrence using the action identifier actions/checkout@v6 and update it to the corresponding commit SHA for that v6 release, appending an inline comment with the original tag (e.g., “# v6”) for clarity; do the same for any other "uses:" entries pinned to tags in this workflow (the review noted another occurrence) so every third-party action is referenced by its commit SHA rather than a tag.
15-15: 💤 Low valueConsider setting
persist-credentials: falsefor defence in depth.The checkout action defaults to persisting GitHub credentials in
.git/config. For CI workflows that execute build tools on untrusted code, explicitly settingpersist-credentials: falseprevents potential credential leakage:- uses: actions/checkout@v6 with: persist-credentials: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/validate.yml at line 15, Update the GitHub Actions checkout step that currently uses actions/checkout@v6 to explicitly disable credential persistence by adding the with: persist-credentials: false option; locate the actions/checkout@v6 usage in the workflow and add a with block containing persist-credentials: false so the checkout step no longer writes GitHub credentials into .git/config.platformio.ini (1)
96-159: ⚖️ Poor tradeoffProtobuf environment duplication creates maintenance burden.
The three protobuf variant environments (
*-proto) fully duplicate their base configurations (~43 lines total). Whilst the comment acknowledges this is necessary becauseplatformio cidoesn't reliably inherit viaextends, this creates a maintenance tax: dependency version updates (e.g., nanopb, opentelemetry_proto) must be applied in six places (3 base + 3 proto).Consider documenting a policy to keep these in sync, or exploring whether newer PlatformIO versions have resolved the
extendslimitation with the CI command.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platformio.ini` around lines 96 - 159, The three protobuf variant environments ([env:esp32dev-proto], [env:rpipicow-proto], [env:esp8266-proto]) duplicate identical dependency and build_flag blocks (notably nanopb/Nanopb and aodtorusan/opentelemetry_proto), creating a maintenance burden; fix by either documenting an explicit sync/update policy in the repo (README or CONTRIBUTING) that calls out these envs and the keys to keep in sync (lib_deps and DOTEL_* build_flags) or, if CI now supports it, consolidate common deps/flags into a single shared env and have the three proto envs extend it—update the comment to state which approach was chosen and list the symbols to update (nanopb/Nanopb, aodtorusan/opentelemetry_proto, and the DOTEL_* build_flags).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/proto_otlp/main.cpp`:
- Around line 154-158: The code is sending an ever-growing totalReadings value
to OTel::Metrics::sum with temporality "DELTA", which double-counts; either send
the per-cycle increment (1.0) instead of totalReadings or change the temporality
to "CUMULATIVE". Update the call site that uses the totalReadings variable and
OTel::Metrics::sum: if you want DELTA, pass the increment (e.g., 1.0) each cycle
and keep isMonotonic=true/"DELTA", or if you intend to export totalReadings keep
the value but change the temporality argument from "DELTA" to "CUMULATIVE".
In `@include/OtelSender.h`:
- Around line 69-74: The current default makes TLS insecure by default via the
OTEL_TLS_INSECURE macro; change the default to secure-by-default by setting
OTEL_TLS_INSECURE to 0 when not defined (flip the `#define` to 0), and ensure any
code that calls WiFiClientSecure::setInsecure() or similar only does so when
OTEL_TLS_INSECURE is explicitly set to 1; update the comment in OtelSender.h
accordingly and verify that certificate validation is enabled unless the user
explicitly defines OTEL_TLS_INSECURE=1 (and that OTEL_TLS_CA_CERT handling
remains unchanged).
In `@src/OtelProtoEncoder.cpp`:
- Around line 53-76: cb_merged_map_attrs currently encodes defaults and call
labels in two passes causing duplicate keys; instead build a single merged map
(copy defaults from MergedMapCtx::defaults then overwrite with entries from
MergedMapCtx::call) and then iterate that merged map once to encode entries
(same approach used by the metric protobuf path); update cb_merged_map_attrs to
materialize this merged map before any pb_encode_* calls and apply the identical
merge-and-encode fix at the other occurrence referenced around lines 278-280.
In `@src/OtelSender.cpp`:
- Around line 258-264: The enqueue_() producer currently advances tail_ when the
ring is full which breaks the SPSC invariant that tail_ is consumer-owned;
change the full-case in enqueue_() so it does not write tail_ but simply treats
the buffer as full and drops the new item (increment drops_ via
drops_.fetch_add(1, std::memory_order_relaxed) and return/fail the enqueue),
leaving tail_ untouched; keep the existing head_/QCAP wrap logic and memory
ordering for successful enqueues, and ensure only the consumer modifies tail_
(do not store to tail_ in enqueue_()).
- Around line 218-246: The fire lambda uses the wrong HTTPClient (it always
operates on the global httpClient_) causing plain-HTTP requests to be sent on an
unrelated client; change fire to take an HTTPClient& (or pointer) parameter and
use that parameter inside (addHeader, POST, end), then call fire(httpClient_)
for the HTTPS path and fire(http) (or fire(wc/http) as appropriate) for the
plain HTTP path; update captures/usages accordingly so begin() and POST()/end()
operate on the same HTTPClient instance (references: fire, httpClient_,
HTTPClient http, http.begin).
In `@test/stubs/Arduino.h`:
- Around line 87-88: The toInt() and toFloat() stubs use std::stoi/std::stof
which throw on invalid input; wrap the conversions in try/catch so they return 0
(int) or 0.f (float) on std::invalid_argument and std::out_of_range to match
Arduino behaviour; update the implementations of toInt() and toFloat() to catch
these exceptions and return the default zero values instead of allowing
exceptions to propagate.
---
Nitpick comments:
In @.github/workflows/validate.yml:
- Line 15: Replace mutable action tags with immutable commit SHAs: find the
occurrence using the action identifier actions/checkout@v6 and update it to the
corresponding commit SHA for that v6 release, appending an inline comment with
the original tag (e.g., “# v6”) for clarity; do the same for any other "uses:"
entries pinned to tags in this workflow (the review noted another occurrence) so
every third-party action is referenced by its commit SHA rather than a tag.
- Line 15: Update the GitHub Actions checkout step that currently uses
actions/checkout@v6 to explicitly disable credential persistence by adding the
with: persist-credentials: false option; locate the actions/checkout@v6 usage in
the workflow and add a with block containing persist-credentials: false so the
checkout step no longer writes GitHub credentials into .git/config.
In `@platformio.ini`:
- Around line 96-159: The three protobuf variant environments
([env:esp32dev-proto], [env:rpipicow-proto], [env:esp8266-proto]) duplicate
identical dependency and build_flag blocks (notably nanopb/Nanopb and
aodtorusan/opentelemetry_proto), creating a maintenance burden; fix by either
documenting an explicit sync/update policy in the repo (README or CONTRIBUTING)
that calls out these envs and the keys to keep in sync (lib_deps and DOTEL_*
build_flags) or, if CI now supports it, consolidate common deps/flags into a
single shared env and have the three proto envs extend it—update the comment to
state which approach was chosen and list the symbols to update (nanopb/Nanopb,
aodtorusan/opentelemetry_proto, and the DOTEL_* build_flags).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: babd0402-1f4d-4030-83b1-1c40ce272773
📒 Files selected for processing (25)
.github/workflows/validate.yml.gitignoreREADME.mdexamples/basic/README.mdexamples/basic/main.cppexamples/direct_otlp/README.mdexamples/direct_otlp/main.cppexamples/proto_otlp/main.cppinclude/OtelDefaults.hinclude/OtelEmbeddedCpp.hinclude/OtelLogger.hinclude/OtelMetrics.hinclude/OtelProtoEncoder.hinclude/OtelSender.hinclude/OtelTracer.hlibrary.jsonplatformio.inisrc/OtelMetrics.cppsrc/OtelProtoEncoder.cppsrc/OtelSender.cpptest/stubs/Arduino.htest/test_otlp/fake_sender.cpptest/test_otlp/fake_sender.htest/test_otlp/project_src.cpptest/test_otlp/test_otlp.cpp
- doPost_: pass HTTPClient& into fire() so plain-HTTP sends don't accidentally POST on the persistent HTTPS client (was a silent no-op) - enqueue_: drop incoming item when queue is full instead of writing tail_ from the producer, which broke the SPSC ownership invariant and could corrupt the queue on RP2040 under concurrent core-1 consumption - cb_merged_map_attrs: materialise a single merged map before encoding so call-site labels override defaults without emitting duplicate keys - proto_otlp example: send increment (1.0) per cycle for DELTA temporality rather than a cumulative total, which double-counted each interval - Arduino.h stub: guard toInt/toFloat against std::stoi/stof exceptions to match Arduino's return-zero-on-failure behaviour Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OtelSender.cpp (1)
218-245:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftSerialise access to the shared HTTPS transport.
httpClient_,tlsClient_, andhttpInit_are process-wide singletons, but this path is still callable from different ESP32 tasks. Two concurrent sends can interleavebegin(),addHeader(),POST(), andend()on the same objects, which will corrupt the request lifecycle or tear down the other caller’s connection. Please put the HTTPS path behind one transport lock, or keep per-call clients on synchronous targets.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OtelSender.cpp` around lines 218 - 245, The HTTPS send path currently uses shared globals (httpClient_, tlsClient_, and ensureHttpInit_) without synchronization, allowing concurrent tasks to interleave calls to begin/addHeader/POST/end; wrap the entire HTTPS branch so only one caller uses the shared transport at a time (e.g., introduce a mutex/lock used by the code that calls ensureHttpInit_(), httpClient_.begin(...), the fire lambda invocation and client.end()), or alternatively avoid shared objects by creating per-call TLS/http clients for the HTTPS path; update the code around the fire lambda invocation and uses of httpClient_, tlsClient_, and ensureHttpInit_ to acquire/release that lock (or switch to per-call instances) to serialize access.
🧹 Nitpick comments (1)
src/OtelProtoEncoder.cpp (1)
59-77: ⚡ Quick winMaterialise the merged label map once in
sendLog().This callback now allocates and fills a fresh
std::mapduring encoding. Because nanopb sizes submessages before writing them,LogRecordattribute callbacks are walked more than once, so every log pays for repeated map copies and heap churn. Merge once insendLog()and feed the result throughcb_map_attrsinstead.Proposed refactor
-struct MergedMapCtx { - const std::map<String,String>* defaults; - const std::map<String,String>* call; -}; - -static bool cb_merged_map_attrs(pb_ostream_t* s, const pb_field_t* f, void* const* arg) { - auto* ctx = *(MergedMapCtx**)arg; - std::map<String, String> merged(*ctx->defaults); - for (const auto& kv : *ctx->call) - merged[kv.first] = kv.second; - - for (const auto& kv : merged) { +static bool cb_map_attrs(pb_ostream_t* s, const pb_field_t* f, void* const* arg) { + auto* ctx = *(MapCtx**)arg; + for (const auto& kv : *ctx->m) { opentelemetry_proto_common_v1_KeyValue entry = opentelemetry_proto_common_v1_KeyValue_init_zero; entry.key.funcs.encode = cb_cstr; entry.key.arg = (void*)kv.first.c_str(); @@ - MergedMapCtx logAttrs{&defaultLabels, &callLabels}; - lr.attributes.funcs.encode = cb_merged_map_attrs; + std::map<String, String> mergedLabels(defaultLabels); + for (const auto& kv : callLabels) + mergedLabels[kv.first] = kv.second; + MapCtx logAttrs{&mergedLabels}; + lr.attributes.funcs.encode = cb_map_attrs; lr.attributes.arg = &logAttrs;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OtelProtoEncoder.cpp` around lines 59 - 77, cb_merged_map_attrs currently allocates and fills a std::map on each nanopb encoding pass, causing repeated heap churn because nanopb may size submessages multiple times; instead, build the merged map once in sendLog() and pass that prebuilt map into the encoder via the existing MergedMapCtx used by cb_map_attrs/cb_merged_map_attrs. Change sendLog() to create the merged std::map<String,String> (merging defaults and call-site labels), store it in MergedMapCtx.call (or add a new pointer field) with a lifetime that outlives the encode calls, and modify cb_merged_map_attrs to only iterate over that prebuilt map (no allocation/copy) and encode entries; keep function names cb_merged_map_attrs, cb_map_attrs, sendLog, and MergedMapCtx as reference points when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/OtelSender.cpp`:
- Around line 218-245: The HTTPS send path currently uses shared globals
(httpClient_, tlsClient_, and ensureHttpInit_) without synchronization, allowing
concurrent tasks to interleave calls to begin/addHeader/POST/end; wrap the
entire HTTPS branch so only one caller uses the shared transport at a time
(e.g., introduce a mutex/lock used by the code that calls ensureHttpInit_(),
httpClient_.begin(...), the fire lambda invocation and client.end()), or
alternatively avoid shared objects by creating per-call TLS/http clients for the
HTTPS path; update the code around the fire lambda invocation and uses of
httpClient_, tlsClient_, and ensureHttpInit_ to acquire/release that lock (or
switch to per-call instances) to serialize access.
---
Nitpick comments:
In `@src/OtelProtoEncoder.cpp`:
- Around line 59-77: cb_merged_map_attrs currently allocates and fills a
std::map on each nanopb encoding pass, causing repeated heap churn because
nanopb may size submessages multiple times; instead, build the merged map once
in sendLog() and pass that prebuilt map into the encoder via the existing
MergedMapCtx used by cb_map_attrs/cb_merged_map_attrs. Change sendLog() to
create the merged std::map<String,String> (merging defaults and call-site
labels), store it in MergedMapCtx.call (or add a new pointer field) with a
lifetime that outlives the encode calls, and modify cb_merged_map_attrs to only
iterate over that prebuilt map (no allocation/copy) and encode entries; keep
function names cb_merged_map_attrs, cb_map_attrs, sendLog, and MergedMapCtx as
reference points when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 46b7d868-6c8d-4adf-9093-f73af40012e9
📒 Files selected for processing (4)
examples/proto_otlp/main.cppsrc/OtelProtoEncoder.cppsrc/OtelSender.cpptest/stubs/Arduino.h
🚧 Files skipped from review as they are similar to previous changes (2)
- test/stubs/Arduino.h
- examples/proto_otlp/main.cpp
cb_merged_map_attrs was allocating and filling a std::map<> inside the nanopb encode callback on every log send. encodeAndSend() calls pb_encode() with a fixed stack buffer (one pass only), so the allocation happened once per send — but doing heap work inside a nanopb callback is unexpected and wasteful on constrained embedded targets. Move the merge into sendLog() before the encode call so the callback receives a fully-built map with no allocation. This lets us reuse the existing cb_map_attrs / MapCtx pair and drop the now-redundant cb_merged_map_attrs and MergedMapCtx entirely. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…U dup OtelProtoEncoder.cpp: all function-local 'static' context structs used as nanopb callback args (DpCtx, SmCtx, RmCtx, DataCtx, SlCtx, RlCtx, LdCtx) are mutated on every call but only need to live until pb_encode() returns. Replace with plain stack variables — cheaper, re-entrant, and no risk of one call overwriting another's context. fillResource()'s ResCtx had the same problem but must outlive the function since pb_encode() is called by the caller after fillResource() returns. Move ResCtx to namespace scope, add it as a caller-owned ref parameter, and update both call sites (buildAndSendNumberMetric, sendLog) accordingly. platformio.ini: exclude OtelMetrics.cpp from the native build_src_filter. test_otlp.cpp includes it directly to keep static-inline singletons in one TU; the build filter was also compiling it as a separate TU, producing duplicate symbol linker errors in the native test build. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
4 tasks
proffalken
added a commit
that referenced
this pull request
Jun 5, 2026
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #15
Problem
PR #15 added HTTPS/TLS support via a persistent
WiFiClientSecure, but theensureHttpInit_()function calledsetHandshakeTimeout()which is only declared in the ESP32'sNetworkClientSecure.h. It is absent from both:WiFiClientSecure(arduino-pico core)WiFiClientSecureThis caused a hard compile error on both non-ESP32 targets:
```
error: 'class BearSSL::WiFiClientSecure' has no member named 'setHandshakeTimeout'
```
Fix
Wrap the
setHandshakeTimeout(20)call in#if defined(ESP32). ThesetTimeout(15)call immediately below is available on all three platforms via the baseClientclass and continues to act as a socket-level connection timeout on ESP8266 and RP2040 — behaviour is functionally equivalent across all targets.Verified
Compiled cleanly against all three environments locally:
esp32devrpipicowesp8266🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores