[io.opentelemetry] Add OTLP metrics and traces - #21219
Conversation
2427dd7 to
caca373
Compare
Extends the logs-only bundle with a full three-signal OTLP/HTTP pipeline: Metrics: OtlpMeterRegistry (Micrometer 1.16.3) attached to openHAB's CompositeMeterRegistry via MeterRegistryProvider. A MeterFilter scopes export to openHAB-own meters (openhab.*, jvm.*, executor.*, process.*, system.*) to avoid exporting unrelated add-on metrics. Supports CUMULATIVE (default) and DELTA aggregation temporality, configurable push interval, and per-signal endpoint override. Traces: SdkTracerProvider with BatchSpanProcessor and OtlpHttpSpanExporter. OpenTelemetryEventListener subscribes to ALL_EVENT_TYPES on the openHAB event bus and emits a flat root span per event (type, topic, source attributes). Sampling ratio is configurable (0.0-1.0). When an OTel Java agent is detected at startup the bundle reuses GlobalOpenTelemetry rather than starting a second SDK instance. All three signals share a single otlpURL + otlpHeaders and a common OTel Resource (service.name=openHAB, service.namespace, service.version, stable service.instance.id, host.name, os.*) for correlated backend entity resolution. Lifecycle hardening: full teardown+rebuild on modified(); bounded forceFlush (3 s) before sdk.close() on deactivate(); own-bundle and OTel-exporter log entries suppressed in the LogListener to prevent export-failure feedback loops; cleartext http:// startup warning; header injection guard (rejects \n/\r/\0); non-positive metricsInterval clamped to default; NaN tracesSamplingRatio falls back to 1.0; event spans always ended via try-finally. Config: per-signal enable toggles (default off), endpoint overrides, metricsInterval (ISO-8601), metricsAggregationTemporality, and tracesSamplingRatio. otlpHeaders masked in logs and toString(). OTel Java agent coexistence: reuses GlobalOpenTelemetry when an agent is present; tracesEnabled description clarifies these are flat single-span event-bus traces, not distributed call trees. README documents protocol/port selection and avoiding JVM metric duplication. Tests: 31 unit tests covering header parsing and injection guard, URL joining, meter-name filter, severity mapping, own-logger suppression, event attribute mapping, null/blank event.source omission, sampling ratio clamping, and secret masking. Signed-off-by: Florian Lettner <florian.lettner@dynatrace.com>
caca373 to
ebb0449
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends the org.openhab.io.opentelemetry add-on from a logs-only OTLP/HTTP exporter to a full three-signal OpenTelemetry pipeline by adding OTLP metrics export (via Micrometer’s OTLP registry) and traces export (event-bus spans via an EventSubscriber). It also updates configuration/UI strings and expands unit test coverage for the new behavior.
Changes:
- Add Micrometer OTLP push metrics pipeline wired into openHAB’s
CompositeMeterRegistry, with meter-name prefix filtering. - Add event-bus span emission via a dynamically registered
EventSubscriber, with optional reuse ofGlobalOpenTelemetrywhen an OTel Java agent is detected. - Update configuration schema/i18n/docs and add/extend unit tests for URL joining, header parsing hardening, filtering, and listener behaviors.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| bundles/org.openhab.io.opentelemetry/src/main/java/org/openhab/io/opentelemetry/internal/OpenTelemetryService.java | Adds metrics + traces initialization/teardown, shared resource attributes, header injection guard, and agent coexistence logic. |
| bundles/org.openhab.io.opentelemetry/src/main/java/org/openhab/io/opentelemetry/internal/OpenTelemetryConfiguration.java | Adds metrics/traces configuration fields and replaces URI.resolve() with a safer URL join for vendor base-path endpoints. |
| bundles/org.openhab.io.opentelemetry/src/main/java/org/openhab/io/opentelemetry/internal/OpenTelemetryEventListener.java | New event-bus subscriber that emits one root span per event with basic attributes. |
| bundles/org.openhab.io.opentelemetry/src/main/java/org/openhab/io/opentelemetry/internal/OpenTelemetryLogListener.java | Adds suppression for bundle/exporter logs to avoid export-failure feedback loops; improves null handling. |
| bundles/org.openhab.io.opentelemetry/src/main/resources/OH-INF/config/config.xml | Adds metrics/traces parameter groups and new configuration parameters/options. |
| bundles/org.openhab.io.opentelemetry/src/main/resources/OH-INF/i18n/io-opentelemetry.properties | Adds i18n strings for new metrics/traces configuration and updates add-on description. |
| bundles/org.openhab.io.opentelemetry/src/main/resources/OH-INF/addon/addon.xml | Updates add-on description to include logs, metrics, and traces. |
| bundles/org.openhab.io.opentelemetry/README.md | Documents new signals, configuration parameters, deployment patterns, and agent coexistence. |
| bundles/org.openhab.io.opentelemetry/doc/otel-collector.md | Updates collector setup documentation for logs/metrics/traces pipelines and modernizes example configs. |
| bundles/org.openhab.io.opentelemetry/doc/lgtm-stack.md | Updates LGTM stack doc to reflect multi-signal support and temporality guidance. |
| bundles/org.openhab.io.opentelemetry/pom.xml | Adds Micrometer OTLP registry dependency and embeds/export-wires OTel proto/protobuf for OSGi. |
| bundles/org.openhab.io.opentelemetry/src/test/java/org/openhab/io/opentelemetry/internal/OpenTelemetryServiceTest.java | Expands tests for header validation, URL joining, meter filtering, config masking, and defaults. |
| bundles/org.openhab.io.opentelemetry/src/test/java/org/openhab/io/opentelemetry/internal/OpenTelemetryLogListenerTest.java | Adds test for suppression of bundle/exporter logger names. |
| bundles/org.openhab.io.opentelemetry/src/test/java/org/openhab/io/opentelemetry/internal/OpenTelemetryEventListenerTest.java | New tests validating event-bus subscription, span creation, attribute mapping, and exception safety. |
Comments suppressed due to low confidence (2)
bundles/org.openhab.io.opentelemetry/README.md:130
- The README lists
metricsEnableddefaulting totrue, but the actual defaults in the add-on config (config.xml) andOpenTelemetryConfigurationarefalse. The documentation table should reflect the real default.
| `metricsEnabled` | Enable exporting openHAB metrics to the OTLP endpoint | `true` |
bundles/org.openhab.io.opentelemetry/README.md:139
- The README lists
tracesEnableddefaulting totrue, but the actual defaults in the add-on config (config.xml) andOpenTelemetryConfigurationarefalse. Please align the documentation default with the shipped configuration.
| `tracesEnabled` | Enable exporting event-bus spans to the OTLP endpoint | `true` |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix README defaults: logsEnabled, metricsEnabled, tracesEnabled were documented as true but ship as false in config.xml and the Java config - Guard the cleartext-HTTP warning behind an anyPipelineEnabled check so a fresh install (all signals disabled by default) emits no spurious warning - Extract clampSamplingRatio() as a package-private static helper so the unit test exercises production code rather than re-implementing the logic locally - Fix spotless formatting in OpenTelemetryConfiguration.toString() Signed-off-by: Florian Lettner <florian.lettner@dynatrace.com>
0a54429 to
f2700d6
Compare
wborn
left a comment
There was a problem hiding this comment.
Thanks for extending the OpenTelemetry add-on with metrics and event-bus traces. The implementation is well documented, includes useful configuration validation, and adds good unit-test coverage. The previously reported issues also appear to have been addressed.
I found three remaining correctness issues that should be fixed before merging:
- OpenTelemetry SDK loggers are no longer fully suppressed, so SDK-side exporter failures can still feed back into the log export pipeline.
- Trace endpoint validation performed by
OtlpHttpSpanExporterBuilder.setEndpoint()can escape the current error handling and fail component activation or reconfiguration. - The metrics filter infers ownership from broad name prefixes, even though openHAB core already identifies its meters using the
openhab_core_metric=truetag.
As a non-blocking cleanup, the Micrometer OTLP registry should preferably use the same Micrometer version as openHAB core rather than pinning 1.16.3 while core uses 1.17.0.
- Broaden log suppression from io.opentelemetry.exporter.* to the full io.opentelemetry.* namespace: SDK batch processors (e.g. BatchLogRecordProcessor) log under io.opentelemetry.sdk.*, so the narrower guard left the export-failure feedback loop open - Guard OtlpHttpSpanExporter.setEndpoint() inside the try block: setEndpoint() validates the scheme and throws IllegalArgumentException for unsupported schemes (e.g. ftp://); the traces path was unguarded while the logs path already handled this correctly - Replace name-prefix meter filter with openhab_core_metric=true tag filter: name prefixes are not exclusive to openHAB core (any add-on can register executor.* or jvm.* meters), and new core meters with different prefixes would be silently excluded; DefaultMetricsRegistration already attaches this tag to every core binder - Update README to reflect tag-based filter boundary - Add regression tests for all three changes Signed-off-by: Florian Lettner <florian.lettner@dynatrace.com>
Bump micrometer-registry-otlp from 1.16.3 to 1.17.0 to match the Micrometer version shipped by openHAB core's io.monitor bundle. Signed-off-by: Florian Lettner <florian.lettner@dynatrace.com>
florian-h05
left a comment
There was a problem hiding this comment.
Thanks for your PR, Florian!
I've added a few first comments -- README and tests haven't been reviewed yet.
Note: Please don't mark comments as resolved. I'll do that.
|
|
||
| /** | ||
| * Tag attached by openHAB core's DefaultMetricsRegistration to every core meter binder. | ||
| * The constant lives in a non-exported core package, so the literal is duplicated here. |
There was a problem hiding this comment.
Ideally, make a small core PR to make them available publicly.
Ff the const is changed in one place, it will likely be forgotten to be changed in its duplicates.
There was a problem hiding this comment.
Agreed on the drift risk. However, I'd rather not block this one on that. I'll file the follow-up core PR and switch to the import once it lands. Let me know if you'd prefer it exposed via org.openhab.core.io.monitor directly or a new accessor method.
There was a problem hiding this comment.
Agreed on not blocking this one here.
Add a new class to org.openhab.core.io.monitor (not the internal sub-package) holding the io.micrometer.core.instrument.Tag as a public static constant.
There was a problem hiding this comment.
Understood.
For now I extended the comment here so the duplicated literal is clearly marked as temporary.
Fix VuePress admonition rendering (:::note is not a registered container), trim over-verbose comments, harden the event-listener registration against double-registration, switch the resource service-instance-id to openHAB's persistent InstanceUUID, centralize OTLP header parsing, and other review-requested cleanups. Signed-off-by: Florian Lettner <florian.lettner@dynatrace.com>
florian-h05
left a comment
There was a problem hiding this comment.
Some final human review comments.
Will probably run an AI review on it as well.
|
|
||
| /** | ||
| * Tag attached by openHAB core's DefaultMetricsRegistration to every core meter binder. | ||
| * The constant lives in a non-exported core package, so the literal is duplicated here. |
There was a problem hiding this comment.
Agreed on not blocking this one here.
Add a new class to org.openhab.core.io.monitor (not the internal sub-package) holding the io.micrometer.core.instrument.Tag as a public static constant.
Co-authored-by: Florian Hotze <dev@florianhotze.com> Signed-off-by: Florian Lettner <Ltty@users.noreply.github.com>
Co-authored-by: Florian Hotze <dev@florianhotze.com> Signed-off-by: Florian Lettner <Ltty@users.noreply.github.com>
Co-authored-by: Florian Hotze <dev@florianhotze.com> Signed-off-by: Florian Lettner <Ltty@users.noreply.github.com>
Signed-off-by: Florian Lettner <florian.lettner@dynatrace.com>
florian-h05
left a comment
There was a problem hiding this comment.
Two minor comments, other then this LGTM.
Co-authored-by: Florian Hotze <dev@florianhotze.com> Signed-off-by: Florian Lettner <Ltty@users.noreply.github.com>
Co-authored-by: Florian Hotze <dev@florianhotze.com> Signed-off-by: Florian Lettner <Ltty@users.noreply.github.com>
florian-h05
left a comment
There was a problem hiding this comment.
LGTM, thanks!
@wborn FYI, I've run your AI review with Antigravity, it didn't find any thing besides what what I already commented on and #21219 (comment), which also has been addressed by now.
wborn
left a comment
There was a problem hiding this comment.
Thanks for the updates. The previously reported correctness issues are fixed in the current revision. There are two remaining issues that should be addressed before merge: the documented trace sampling configuration is ignored when event-bus spans are routed through the OTel Java agent, and the newly added third-party dependencies are not yet reflected in NOTICE.
This review was AI-assisted.
Document that tracesSamplingRatio has no effect when the OTel Java agent supplies the tracer, and how to configure sampling on the agent instead. Add NOTICE entries for the Micrometer, OpenTelemetry Proto, and protobuf-java dependencies added by this PR. Signed-off-by: Florian Lettner <florian.lettner@dynatrace.com>
wborn
left a comment
There was a problem hiding this comment.
The findings from the previous review are addressed in the current revision: Java-agent sampling behavior is now documented and the added runtime dependencies are covered by NOTICE. The complete updated change was reviewed again and CI is successful.
One remaining non-blocking Java-agent detection edge case is noted inline.
This review was AI-assisted.
|
Besides the review, there is a DCO issue: |
Detecting the OTel Java agent via a JVM argument substring match is unreliable: it misses a renamed agent JAR, and it false-positives when the agent is attached but disabled, silently dropping spans against a no-op GlobalOpenTelemetry. Check GlobalOpenTelemetry.isSet() instead, which reflects whether a usable global is actually installed. Signed-off-by: Florian Lettner <florian.lettner@dynatrace.com>
63a5a3f to
ecbdd4e
Compare
Description
This is a follow-up to #21127 (initial logs-only contribution), extending the OpenTelemetry service with a full three-signal OTLP/HTTP pipeline - logs (already in #21127), metrics, and traces.
Metrics
A Micrometer
OtlpMeterRegistry(Micrometer 1.16.3) is attached to openHAB'sCompositeMeterRegistryviaMeterRegistryProvider. AMeterFilterscopes export to openHAB-own meters (openhab.*,jvm.*,executor.*,process.*,system.*,logback.*,http.*) to avoid leaking unrelated add-on metrics.Configuration:
metricsEnabled,metricsEndpoint,metricsInterval(ISO-8601 duration), andmetricsAggregationTemporality(CUMULATIVE/DELTA- required for some backends such as Dynatrace).Traces (event-bus spans)
A
SdkTracerProviderwithBatchSpanProcessorandOtlpHttpSpanExporteris wired to anOpenTelemetryEventListenerthat subscribes toALL_EVENT_TYPESon the openHAB event bus. Each event produces a flat root span (event.type,event.topic,event.sourceattributes). Spans are root-level by design - the event bus is fire-and-forget with no propagated call context.When an OTel Java agent is detected at JVM startup the bundle reuses
GlobalOpenTelemetryinstead of starting a competing SDK instance.Configuration:
tracesEnabled,tracesEndpoint,tracesSamplingRatio(0.0-1.0, clamped, NaN-safe).Lifecycle hardening
forceFlush(3 s) beforesdk.close()on deactivate/reconfigureparseOtlpHeaders()- rejects\n,\r,\0OpenTelemetryLogListenerto prevent export-failure feedback loopshttp://warning on startupmetricsIntervalclamped to default (PT60S); NaNtracesSamplingRatiofalls back to 1.0OSGi dependency notes
micrometer-registry-otlpimportsio.opentelemetry.proto.*. We embedopentelemetry-proto:1.8.0-alphaandprotobuf-java:4.32.0privately, and exportio.opentelemetry.proto.*with-noimport:=trueso the micrometer bundle can wire to them. All other OTel SDK classes stay private. The resulting JAR is self-contained.Shared resource / signal correlation
All three signals share a single
otlpURL/otlpHeadersand a common OTelResource(service.name=openHAB,service.namespace,service.version, stableservice.instance.id,host.name,os.*) so logs, metrics, and traces correlate to a single service entity in the observability backend.Tests
31 unit tests added covering: header parsing and injection guard, URL joining (including vendor base-path regression), meter-name filter, log severity mapping, own-logger suppression, event attribute mapping, null/blank
event.sourceomission, sampling-ratio clamping, and secret masking intoString().Testing
Gate build: BUILD SUCCESS - 31/31 tests, 0 failures.
The three signals have been verified end-to-end against a Dynatrace OTLP ingest with both an OTel Collector and direct-to-backend configurations.