CAMEL-24051: Add observability test-infra module#24689
Conversation
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 15 tested, 26 compile-only — current: 12 all testedMaveniverse Scalpel detected 41 affected modules (current approach: 12).
|
2167c55 to
09fede2
Compare
New test-infra module composing Prometheus, VictoriaTraces, VictoriaLogs, and Perses for local observability via `camel infra run observability`. - Prometheus scrapes Camel metrics at :9876/observe/metrics (zero-config with --observe) - VictoriaTraces receives OTLP traces at :10428 - VictoriaLogs receives OTLP logs at :9428 - Perses auto-provisioned with Camel project and overview dashboard at :3000 - New --open-telemetry-agent-export=observability target for traces+logs - Deprecated --open-telemetry-agent-export=jaeger in favor of otlp - Generalized OpenTelemetryTracer.exportTarget check to accept any value - Fixed fork version propagation in Run.java for camel run with OTel agent
- Fix: observability export now treated as external in Run.java (no dead embedded receiver) - Fix: set camel.opentelemetry2.exportTarget=observability in fork path - Revert: fork version propagation change (separate concern, separate JIRA) - Use mirror.gcr.io instead of docker.io for container images - Add upgrade guide entry for export target changes and new infra command
09fede2 to
7f4fe80
Compare
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Claude Code on behalf of gnodet
Review — CAMEL-24051: Add observability test-infra module
Nice work, Federico! This adds a well-designed camel-test-infra-observability module composing four containers (Prometheus, VictoriaTraces, VictoriaLogs, Perses) into a local observability stack, along with the CLI wiring (camel run --observability) to use it.
What looks great
-
Clean export-mode decomposition in
Run.java— The three-way split betweenobservability(traces + logs),otlp(traces only, external), andtui(embedded) is well-reasoned. Each mode configures exactly the right set of OTel SDK system properties — notably,otel.logs.exporter=otlpis only set for the observability path. -
Smart generalization of
OpenTelemetryTracer.exportTarget— Replacing"jaeger".equals(exportTarget)with a null/empty check is forward-compatible. Any future export target works without touching this class again, andtuimode (which never setsexportTarget) is unaffected. -
Good adherence to project container-image conventions — Using
quay.ioandmirror.gcr.ioper project rules. ThewithExtraHost("host.docker.internal", "host-gateway")on Prometheus correctly handles Linux Docker environments.
Items to address
1. FQCN usage in ObservabilityLocalContainerInfraService.java
The loadResource method uses java.nio.charset.StandardCharsets.UTF_8 as a fully qualified class name. Project rules require an import + simple name. The CI OpenRewrite check will flag this and fail the build with uncommitted changes.
// Current:
return new String(is.readAllBytes(), java.nio.charset.StandardCharsets.UTF_8);
// Fix — add import and use simple name:
import java.nio.charset.StandardCharsets;
return new String(is.readAllBytes(), StandardCharsets.UTF_8);(Note: PrometheusContainer.java already imports and uses StandardCharsets correctly.)
2. Multi-arch container image availability — missing skipITs properties
Per project guidelines: "Verify that the container image is available for the tested architectures (currently amd64, ppc64le and s390x). If not available, use skipITs.ppc64le and skipITs.s390x Maven properties."
- Perses (
persesdev/perses:v0.53.1): onlyamd64andarm64— noppc64le, nos390x - VictoriaTraces/VictoriaLogs:
amd64,arm,arm64,ppc64le,386— nos390x - Prometheus: full coverage
The pom.xml should add:
<properties>
<skipITs.ppc64le>true</skipITs.ppc64le>
<skipITs.s390x>true</skipITs.s390x>
</properties>Without this, CI on those architectures will fail when trying to pull images.
3. Network resource leak safety in shutdown()
If any container's stop() throws (e.g., the container crashed or Docker daemon is unreachable), subsequent container stops and network.close() will be skipped. Consider wrapping each stop in try-catch:
@Override
public void shutdown() {
LOG.info("Stopping observability stack");
for (var action : List.of(
(Runnable) persesContainer::stop,
victoriaLogsContainer::stop,
victoriaTracesContainer::stop,
prometheusContainer::stop,
network::close)) {
try {
action.run();
} catch (Exception e) {
LOG.warn("Shutdown step failed: {}", e.getMessage());
}
}
}Minor suggestions
-
serviceVersion: nullin generatedmetadata.json— Other entries use actual version strings or"latest". Since this is a multi-container module,nullmay be expected, but it could confuse catalog API consumers. Consider whether the generator can emit"latest"or a composite version. -
Hardcoded OTLP endpoint ports in
Run.java— The observability export path hardcodes Victoria port numbers (10428,9428). These matchObservabilityPropertiesdefaults, but extracting them as named constants would make the coupling explicit. -
Property naming inconsistency —
observability.victoria.traces.port(dot separator) vsobservability.victoriatraces.container(no dot). Minor but could confuse users overriding container image names.
CI status
| Check | Status |
|---|---|
| build | ✅ pass (3m45s) |
| build (17, false) | ⏳ pending |
| build (25, false) | ⏳ pending |
Overall this is a solid addition to the test-infra family. The items above are all addressable without rethinking the design.
- Add uiSupported=true to @infraservice annotation - Add uiUrl() default method returning Perses dashboard URL - Fix: add observability to externalExport check in Run.java - Fix: set camel.opentelemetry2.exportTarget=observability in fork path
b848400 to
64e62cc
Compare
|
There are uncommitted changes |
- Fix FQCN: import StandardCharsets instead of inline fully qualified name - Add skipITs.ppc64le and skipITs.s390x (Perses/Victoria images lack these archs) - Safe shutdown: wrap each container stop in try-catch to prevent resource leaks
0926d71 to
991e494
Compare
3 participants
Summary
Claude Code on behalf of Croway
New
camel-test-infra-observabilitymodule providing a local Apache-2.0-only observability stack viacamel infra run observability.Composes 4 containers on a shared Docker network:
:9090) — scrapes Camel metrics at:9876/observe/metrics:10428) — receives OTLP traces:9428) — receives OTLP logs:3000) — dashboard UI, auto-provisioned with Camel project and overview dashboardChanges
test-infra/camel-test-infra-observability/with 4 container classes, service interface, multi-container orchestrator--open-telemetry-agent-export=observabilityenables traces + logs export to the stack--open-telemetry-agent-export=jaegerin favor ofotlp(backward compatible)OpenTelemetryTracer.exportTargetcheck accepts any non-empty value instead of hardcoded"jaeger"Run.java— forked JBang processes now inherit the CLI's catalog versionUsage
Verified
--observe)--management-portTest plan
camel infra run observabilitystarts all 4 containerscamel infra get observabilityreturns JSON with all endpoint URLscamel run --observesends metrics to Prometheuscamel run --observe --open-telemetry-agent --open-telemetry-agent-export=observabilitysends traces and logs:3000/projects/camel/dashboards/overviewshows datacamel infra stop observabilitystops all containers--open-telemetry-agent-export=jaegerstill works (backward compatible)--open-telemetry-agent-export=otlpworks as new generic external export