Repository navigation
Conversation
Signed-off-by: foukou19 <ferial.oukoukas@decathlon.com>
Signed-off-by: foukou19 <ferial.oukoukas@decathlon.com>
Signed-off-by: foukou19 <ferial.oukoukas@decathlon.com>
Signed-off-by: foukou19 <ferial.oukoukas@decathlon.com>
Co-authored-by: Eve Bernhard <115796475+evebrnd@users.noreply.github.com> Signed-off-by: ferial OUKOUKAS <75682459+foukou19@users.noreply.github.com>
Signed-off-by: foukou19 <ferial.oukoukas@decathlon.com>
Signed-off-by: foukou19 <ferial.oukoukas@decathlon.com>
Signed-off-by: foukou19 <ferial.oukoukas@decathlon.com>
Signed-off-by: foukou19 <ferial.oukoukas@decathlon.com>
3229034 to
4bad155
Compare
Code coverage overviewLanguages: Java Java / code-coverage/jacoco
View coverage of the 10 most impacted files.
ℹ️ Calculations represent line coverage. Results may change as calculations are updated. Updated |
Signed-off-by: foukou19 <ferial.oukoukas@decathlon.com>
Signed-off-by: ferial OUKOUKAS <75682459+foukou19@users.noreply.github.com>
Signed-off-by: foukou19 <ferial.oukoukas@decathlon.com> Signed-off-by: foukou19 <ferial.oukoukas@decathlon.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved metric accuracy and label-cardinality issues undermine reliable monitoring.
5 open findings
Limit mapping catch to mapping-level failure metrics · New Avoid double-counting and misclassifying entity validation failures · New Pass resolved connector identifier explicitly to decoding metrics · New Record oversized payload decoding errors exactly once · New Bound encoding metric labels to prevent cardinality growth · New
What changed in this PR
Adds OpenTelemetry metrics to idp-core’s webhook ingestion pipeline.
Changes:
- Instruments security validation, payload decoding, mapping, and entity operations.
- Adds shared metric constants, helpers, and meter configuration.
- Updates test setup and telemetry dependencies.
| File | Description |
|---|---|
| src/test/java/com/decathlon/idp_core/infrastructure/adapters/ingestion/processor/IngestionProcessorTest.java | Supplies a test meter. |
| src/test/java/com/decathlon/idp_core/infrastructure/adapters/ingestion/processor/DecodingProcessorTest.java | Updates constructors with a meter. |
| src/main/java/com/decathlon/idp_core/infrastructure/adapters/ingestion/processor/SecurityProcessor.java | Records security outcomes. |
| src/main/java/com/decathlon/idp_core/infrastructure/adapters/ingestion/processor/IngestionProcessor.java | Records mapping and entity metrics. |
| src/main/java/com/decathlon/idp_core/infrastructure/adapters/ingestion/processor/decoder/DecodingProcessor.java | Records decoding outcomes and payload sizes. |
| src/main/java/com/decathlon/idp_core/infrastructure/adapters/ingestion/metrics/IngestionObservabilitySupport.java | Adds shared instrumentation helpers. |
| src/main/java/com/decathlon/idp_core/infrastructure/adapters/ingestion/configuration/IngestionObservabilityConstants.java | Defines metric names and attributes. |
| src/main/java/com/decathlon/idp_core/infrastructure/adapters/ingestion/configuration/IngestionMetricsConfiguration.java | Configures scoped meters. |
| pom.xml | Adds telemetry API and testing dependencies. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
The new OpenTelemetry meters are not connected to the configured metrics export path.
6 open findings
Connect OpenTelemetry meters to the configured metrics exporter · New Bound encoding metric labels to prevent cardinality growth Record oversized payload decoding errors exactly once Pass resolved connector identifier explicitly to decoding metrics Avoid double-counting and misclassifying entity validation failures Limit mapping catch to mapping-level failure metrics
🧠 Review effort: Balanced
072443b to
33e8861
Compare
Signed-off-by: foukou19 <ferial.oukoukas@decathlon.com>
33e8861 to
d55eb94
Compare
93cdcad to
1226447
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The metrics endpoint is exposed without authentication, allowing unauthenticated callers to enumerate internal meter names, tags, and operational measurements.
11 open issues
Use protocol-aware fallback OTLP endpoints · New Disable Micrometer OTLP export in the test profile · New Restrict public access to the metrics endpoint Use a sampler that honors the configured sampling ratio Disable tracing in the local profile with the new setting Make verbose log levels opt-in for deployments Scope metric caches to the Meter or provider Make per-webhook diagnostic logs opt-in at DEBUG Document the separate Micrometer OTLP export switch · New Replace the nonexistent test command with valid verification · New Align documented security labels with emitted values · New
1 resolved since last review
Review effort: Balanced
| metrics: | ||
| endpoint: ${OTEL_EXPORTER_OTLP_METRICS_ENDPOINT:${OTEL_EXPORTER_OTLP_ENDPOINT:http://localhost:4318}/v1/metrics} | ||
| protocol: ${OTEL_EXPORTER_OTLP_METRICS_PROTOCOL:${OTEL_EXPORTER_OTLP_PROTOCOL:http/protobuf}} | ||
| traces: | ||
| endpoint: ${OTEL_EXPORTER_OTLP_TRACES_ENDPOINT:${OTEL_EXPORTER_OTLP_ENDPOINT:http://localhost:4318}/v1/traces} | ||
| protocol: ${OTEL_EXPORTER_OTLP_TRACES_PROTOCOL:${OTEL_EXPORTER_OTLP_PROTOCOL:http/protobuf}} |
Signed-off-by: foukou19 <ferial.oukoukas@decathlon.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Unauthenticated metrics exposure, instrument caches that misdirect measurements across providers, and a sampler that ignores the configured ratio introduce security and telemetry-correctness regressions.
11 open issues
Disable Micrometer OTLP export in the test profile Use protocol-aware fallback OTLP endpoints Restrict public access to the metrics endpoint Use a sampler that honors the configured sampling ratio Disable tracing in the local profile with the new setting Make verbose log levels opt-in for deployments Scope metric caches to the Meter or provider Make per-webhook diagnostic logs opt-in at DEBUG Align documented security labels with emitted values Replace the nonexistent test command with valid verification Document the separate Micrometer OTLP export switch
Review effort: Balanced
169d699 to
b8ec7cb
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Enabling the metrics endpoint exposes route identifiers and operational measurements without authentication because the public filter chain permits all /actuator/** paths.
10 open issues
Disable Micrometer OTLP export in the test profile Use protocol-aware fallback OTLP endpoints Use a sampler that honors the configured sampling ratio Disable tracing in the local profile with the new setting Make verbose log levels opt-in for deployments Scope metric caches to the Meter or provider Make per-webhook diagnostic logs opt-in at DEBUG Align documented security labels with emitted values Replace the nonexistent test command with valid verification Document the separate Micrometer OTLP export switch
1 resolved since last review
Review effort: Balanced
…s export Signed-off-by: foukou19 <ferial.oukoukas@decathlon.com>
a4c5b0e to
59bc7d0
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Global metric caches can send measurements to the wrong provider, while sampling and logging defaults bypass existing production volume controls.
11 open issues
Record MATCHED only after the entity action succeeds · New Avoid copying byte-array payloads to measure their size · New Assert collected metrics in processor tests · New Use protocol-aware fallback OTLP endpoints Use a sampler that honors the configured sampling ratio Disable tracing in the local profile with the new setting Make verbose log levels opt-in for deployments Scope metric caches to the Meter or provider Make per-webhook diagnostic logs opt-in at DEBUG Align documented security labels with emitted values Replace the nonexistent test command with valid verification
2 resolved since last review
Review effort: Balanced
1333d0f to
988541a
Compare
Signed-off-by: foukou19 <ferial.oukoukas@decathlon.com>
988541a to
00b757a
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unsupported logging properties leave trace correlation fields empty, infrastructure failures are counted as validation errors, and default-on webhook diagnostics add unnecessary logging and processing overhead.
7 open issues
Exclude infrastructure failures from validation-failure counter · New Make tracing diagnostics opt-in · New Use supported OpenTelemetry MDC property names · New Use protocol-aware fallback OTLP endpoints Use a sampler that honors the configured sampling ratio Disable tracing in the local profile with the new setting Update deployment guides for exporter and sampling settings · New
8 resolved since last review
Assert collected metrics in processor tests Avoid copying byte-array payloads to measure their size Record MATCHED only after the entity action succeeds Make verbose log levels opt-in for deployments Scope metric caches to the Meter or provider Make per-webhook diagnostic logs opt-in at DEBUG Align documented security labels with emitted values Replace the nonexistent test command with valid verification
Review effort: Balanced
| incrementCounter(IngestionObservabilityConstants.FUNCTIONAL_VALIDATION_FAILED_TOTAL, | ||
| ATTRIBUTE_CONNECTOR, connectorIdentifier, ATTRIBUTE_TEMPLATE, templateIdentifier, | ||
| ATTRIBUTE_REASON, exception.getClass().getSimpleName()); |
| max-page-size: ${MAX_PAGE_SIZE:500} | ||
| ingestion: | ||
| # Enable temporary diagnostics to inspect Camel exchange telemetry context propagation. | ||
| tracing-diagnostic-enabled: ${APP_INGESTION_TRACING_DIAGNOSTIC_ENABLED:true} |
| trace-id-key: traceId | ||
| span-id-key: spanId |
| is ignored. Set `OTEL_TRACES_SAMPLER` to a ratio-based sampler if you want the | ||
| argument to control sampling. | ||
|
|
||
| Replace the old `OTEL_TRACING_ENABLED` and `OTEL_METRICS_EXPORT_ENABLED` flags with exporter selection (`otlp` or `none`). Replace `*_TRANSPORT=http` with `*_PROTOCOL=http/protobuf`. The official starter reads `otel.*`, not the former `management.opentelemetry.*` export settings. The test profile disables network export for all signals. |
brandPittCode
left a comment
There was a problem hiding this comment.
Test coverage does not guard the regression fixed by this PR
Currently only 3 mapping tests in IngestionProcessorTest.java:173-253 assert metric output.
Add an autoconfiguration test verifying that the Spring context wires a non-noop Meter (preventing regressions back to DefaultMeter), and add InMemoryMetricReader / span assertions for:
Entity lifecycle & failure counters (ENTITY_CREATED_TOTAL, ENTITY_UPDATED_TOTAL, ENTITY_PATCH_TOTAL, ENTITY_DELETED_TOTAL, ENTITY_NOT_FOUND_TOTAL, ENTITY_CONFLICT_TOTAL, ENTITY_VALIDATION_ERRORS_TOTAL, FUNCTIONAL_VALIDATION_FAILED_TOTAL).
Security counters and span attributes in SecurityProcessor.java:46-93 (currently no dedicated SecurityProcessorTest exists).
Payload size, decoded, and decode-error metrics in DecodingProcessorTest.java:28-41.
|
|
||
| @Configuration | ||
| @Slf4j | ||
| public class CamelTracingConfiguration { |
There was a problem hiding this comment.
Add class markdown JavaDoc
| import io.opentelemetry.api.metrics.Meter; | ||
|
|
||
| @Configuration | ||
| public class IngestionMetricsConfiguration { |
There was a problem hiding this comment.
Add class markdown JavaDoc
| import io.opentelemetry.api.metrics.Meter; | ||
| import io.opentelemetry.api.trace.Span; | ||
|
|
||
| public final class IngestionObservabilitySupport { |
There was a problem hiding this comment.
Add class and methods markdown JavaDoc
| incrementCounter(IngestionObservabilityConstants.FUNCTIONAL_VALIDATION_FAILED_TOTAL, | ||
| ATTRIBUTE_CONNECTOR, connectorIdentifier, ATTRIBUTE_TEMPLATE, templateIdentifier, | ||
| ATTRIBUTE_REASON, exception.getClass().getSimpleName()); |
| Span.current().setAttribute(SPAN_ATTRIBUTE_SECURITY_RESULT, "REJECTED"); | ||
| String result = isSecurityRejected(exception) ? RESULT_REJECTED : RESULT_ERROR; | ||
| recordSecurityResult(webhookIdentifier, securityType, result); | ||
| incrementCounter(IngestionObservabilityConstants.SECURITY_VALIDATION_ERRORS_TOTAL, |
There was a problem hiding this comment.
Maybe add more specific name. Like incrementSecuritityValidationErrorsCounter
| org.apache.camel.opentelemetry: ${CAMEL_OTEL_DIAGNOSTIC_LOG_LEVEL:WARN} | ||
| io.opentelemetry.exporter: ${OTEL_EXPORTER_DIAGNOSTIC_LOG_LEVEL:WARN} | ||
| io.opentelemetry.exporter.sender.okhttp: ${OTEL_EXPORTER_HTTP_DIAGNOSTIC_LOG_LEVEL:WARN} | ||
| khttp3: ${OKHTTP_DIAGNOSTIC_LOG_LEVEL:WARN} |
There was a problem hiding this comment.
khttp3 is missing the leading o (okhttp3), so OKHTTP_DIAGNOSTIC_LOG_LEVEL never takes effect.
|
|
||
| static final String CAMEL_INSTRUMENTATION_SCOPE = "camel"; | ||
|
|
||
| @Bean |
There was a problem hiding this comment.
Null-safe OpenTelemetry fallback in Camel tracing
In CamelTracingConfiguration.java:20-29, pass OpenTelemetry::noop as the default supplier so sliced tests or contexts without the OTel starter do not fail with a NullPointerException:
| @Bean | |
| @Bean | |
| public Tracer camelTracer(ObjectProvider<OpenTelemetry> openTelemetryProvider) { | |
| return openTelemetryProvider.getIfAvailable(OpenTelemetry::noop) | |
| .getTracer(CAMEL_INSTRUMENTATION_SCOPE); | |
| } | |
| @Bean | |
| public ContextPropagators camelContextPropagators( | |
| ObjectProvider<OpenTelemetry> openTelemetryProvider) { | |
| return openTelemetryProvider.getIfAvailable(OpenTelemetry::noop).getPropagators(); | |
| } |
|
|
||
| public final class IngestionObservabilitySupport { | ||
|
|
||
| private static final Map<InstrumentCacheKey, LongCounter> COUNTER_CACHE = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Static instrument cache retains closed Meter instances
IngestionObservabilitySupport.java:24-25 stores counters and histograms in static final ConcurrentHashMaps keyed by Meter identity. Every new SdkMeterProvider created in unit tests or context reloads remains strongly referenced for the JVM lifetime.
Because IngestionProcessor, SecurityProcessor, and DecodingProcessor are singleton @components, building and storing their LongCounter / LongHistogram instruments as instance fields in the constructor removes the static leak, runtime cache lookups, and per-event String... allocations—and lets you attach .setUnit(...) and .setDescription(...) metadata (ms, By).
Replace static Meter caches with constructor-owned instruments
Instead of caching instruments in static final ConcurrentHashMaps inside IngestionObservabilitySupport.java:24-45, build the instruments once in each singleton processor's constructor (with units and descriptions) and keep IngestionObservabilitySupport stateless.
Example for SecurityProcessor.java:37-44:
private final List<WebhookSecurityStrategy> strategies;
private final LongCounter securityRequestsCounter;
private final LongCounter securityResultCounter;
private final LongCounter securityValidationErrorsCounter;
public SecurityProcessor(List<WebhookSecurityStrategy> strategies,
@Qualifier("securityMeter") Meter meter) {
this.strategies = List.copyOf(strategies);
this.securityRequestsCounter = meter.counterBuilder(SECURITY_REQUESTS_TOTAL)
.setDescription("Number of security checks executed for a connector")
.build();
this.securityResultCounter = meter.counterBuilder(SECURITY_RESULT)
.setDescription("Result of the webhook security decision")
.build();
this.securityValidationErrorsCounter = meter.counterBuilder(SECURITY_VALIDATION_ERRORS_TOTAL)
.setDescription("Security validation failures and malformed configuration errors")
.build();
}
And recording measurements directly with typed AttributeKeys avoids per-request String... vararg parsing and cache lookups:
securityRequestsCounter.add(1, Attributes.of( CONNECTOR_KEY, webhookIdentifier, SECURITY_TYPE_KEY, securityType));
(Apply the same constructor pattern to DecodingProcessor with .setUnit("By") on PAYLOAD_SIZE_BYTES, and remove COUNTER_CACHE, HISTOGRAM_CACHE, and InstrumentCacheKey from IngestionObservabilitySupport.java:24-133.)
| public static final String ENTITY_NOT_FOUND_TOTAL = "idp.webhook.entity.notfound.total"; | ||
| public static final String ENTITY_CONFLICT_TOTAL = "idp.webhook.entity.conflict.total"; | ||
|
|
||
| public static final String ATTRIBUTE_CONNECTOR = "connector"; |
There was a problem hiding this comment.
Duplicated constants and raw attribute keys: SPAN_ATTRIBUTE_WEBHOOK_IDENTIFIER is defined in IngestionObservabilityConstants.java:37 and redeclared privately in both SecurityProcessor.java:31 and IngestionProcessor.java:33. Keys like "error_type" and "encoding" are also inline literals rather than shared constants.
Centralize constants and typed AttributeKeys
Update IngestionObservabilityConstants.java:29-38 so processors do not redeclare SPAN_ATTRIBUTE_* or inline "encoding" / "error_type":
public static final String ATTRIBUTE_CONNECTOR = "connector";
public static final String ATTRIBUTE_MAPPING_ID = "mapping_id";
public static final String ATTRIBUTE_ACTION = "action";
public static final String ATTRIBUTE_REASON = "reason";
public static final String ATTRIBUTE_TEMPLATE = "template";
public static final String ATTRIBUTE_RESULT = "result";
public static final String ATTRIBUTE_SECURITY_TYPE = "security_type";
public static final String ATTRIBUTE_ENCODING = "encoding";
public static final String ATTRIBUTE_ERROR_TYPE = "error_type";
public static final AttributeKey<String> CONNECTOR_KEY = AttributeKey.stringKey(ATTRIBUTE_CONNECTOR);
public static final AttributeKey<String> MAPPING_ID_KEY = AttributeKey.stringKey(ATTRIBUTE_MAPPING_ID);
public static final AttributeKey<String> ACTION_KEY = AttributeKey.stringKey(ATTRIBUTE_ACTION);
public static final AttributeKey<String> REASON_KEY = AttributeKey.stringKey(ATTRIBUTE_REASON);
public static final AttributeKey<String> TEMPLATE_KEY = AttributeKey.stringKey(ATTRIBUTE_TEMPLATE);
public static final AttributeKey<String> RESULT_KEY = AttributeKey.stringKey(ATTRIBUTE_RESULT);
public static final AttributeKey<String> SECURITY_TYPE_KEY = AttributeKey.stringKey(ATTRIBUTE_SECURITY_TYPE);
public static final AttributeKey<String> ENCODING_KEY = AttributeKey.stringKey(ATTRIBUTE_ENCODING);
public static final AttributeKey<String> ERROR_TYPE_KEY = AttributeKey.stringKey(ATTRIBUTE_ERROR_TYPE);
public static final String SPAN_ATTRIBUTE_WEBHOOK_IDENTIFIER = "idp.webhook.identifier";
public static final String SPAN_ATTRIBUTE_SECURITY_RESULT = "idp.webhook.security.result";
Then delete the duplicate private static final String SPAN_ATTRIBUTE_* fields in SecurityProcessor.java:31-32 and IngestionProcessor.java:33.
| log.debug("Mapping filter excluded payload for template: {}", | ||
| mapping.entityTemplateIdentifier()); | ||
| return; | ||
| private void applyMapping(String payload, EntityDynamicMapping mapping, |
There was a problem hiding this comment.
Fix unsupported action outcome, ENTITY_NOT_FOUND_TOTAL action label, and sub-ms latency in IngestionProcessor
Build a DoubleHistogram (.setUnit("ms")) instead of LongHistogram so sub-millisecond mappings are not truncated to 0 (MetricData.getHistogramData() in IngestionProcessorTest.java:148-154 continues to work unchanged).
Throw on case null, default so an invalid action is recorded as ERROR in the catch block rather than falling through to MATCHED.
Pass action to recordEntityFailureMetrics instead of hardcoding "ingestion".
|





fix(metrics,logging,docs): add and harden webhook custom metrics with OTel runtime alignment
What this PR Provides
Migrates observability configuration from legacy
management.*tracing/export keys to nativeotel.*runtime keys.Keeps Camel tracing through
camel-opentelemetry2-starterand the Camel/OpenTelemetry bridge.Adds and stabilizes webhook custom metrics (
idp.webhook.*) emitted through the native OpenTelemetryMeter.Aligns observability documentation with emitted metric labels, available tests, and effective runtime behavior.
ADR link: N/A
Fixes
Review
The reviewer must double-check these points:
!after the type/scope to identify the breakingchange in the release note and ensure we will release a major version.
Breaking changes (if any)