feat: emit OTel log signals on unrouted requests — GH#4705 - #4725
feat: emit OTel log signals on unrouted requests — GH#4705#4725balhar-jakub wants to merge 5 commits into
Conversation
…lRequestContext Add three new OTel attributes for richer error signal logging: - statusCode(int) -> http.response.status_code - errorType(String) -> error.type - errorMessage(String) -> error.message Existing responseCode(int) -> service.response_code is preserved. Implements #4705 Area A (gateway-service module)
github-project-automation
Bot
moved this to New
in API Mediation Layer Backlog Management
Architectural Review — PR #4725 (Area A)Verdict: APPROVED ✅ SummaryArea A of #4705 — adding three new OTel log signal attributes to Review Checklist
Detailed Review1. Constants (lines 51-53)
2. Methods (lines 94-104)
3. Tests (lines 90-105 of test file)
All three follow the existing test pattern ( No Issues Found
Downstream NotesArea B ( otelContext.statusCode(404)
.errorType("Service not onboarded")
.errorMessage("Service " + attemptedService + " is not registered"); |
- Add .doOnError() hooks for NoResourceFoundException (404) and ServiceNotAccessibleException (503) - Fix timing bug: move responseCode setting into doFinally before issue() - Add unit tests for both error scenarios
Document http.response.status_code, error.type, and error.message attributes in otel/README.md. Add error scenario descriptions for Unknown Service ID (404) and Service Instances Down (503) cases, with Prometheus/Alertmanager alerting examples.
Architectural Review — Area B (OtelRequestFilter, commit f6e9f44)Status: APPROVED ✅SummaryArea B extends 1. Design Compliance ✅The implementation follows the Step 2 design exactly:
2. Timing Fix Correctness ✅Before (bug): .doFinally(signalType -> otelContext.issue()) // log issued FIRST
.then(Mono.fromRunnable(() -> ... responseCode ...)) // responseCode set AFTERAfter (fixed): .doFinally(signalType -> {
if (signalType == SignalType.ON_COMPLETE) {
// set responseCode from actual response BEFORE issue()
Optional.ofNullable(exchange.getResponse())
.map(ServerHttpResponse::getStatusCode)
.map(HttpStatusCode::value)
.ifPresent(otelContext::responseCode);
}
otelContext.issue(); // log now includes responseCode
});The
3. Reactive Chain Structure ✅
4. Error Message Construction ✅
5. Backward Compatibility ✅
6. Test Coverage ✅
7. Potential Risks (Low)
VerdictClean implementation. No structural concerns. Ready for CI validation. — Architect |
balhar-jakub
changed the title
Architectural Review — PR #4725 for #4705Reviewer: Architect (Hermes pipeline — Steps 5-6) Design Compliance ✅All implementation decisions match the design specification:
Structural Integrity ✅
API Contracts ✅
Test Coverage ✅
Documentation ✅
Verdict: APPROVED ✅No architectural concerns. Implementation is a clean, minimal extension of existing infrastructure. Ready for CI validation. Hermes pipeline: architect Steps 5-6 — t_383a1bcb |
SonarCloud - e.getMessage() can return null, but OtelRequestContext.put() passes the value directly to AttributesBuilder.put() which throws NPE on null. Use Objects.toString() with a descriptive fallback value.
Signed-off-by: Jakub Balhar <jakub@balhar.net>
|
EvaJavornicka
moved this from New
to In Progress
in API Mediation Layer Backlog Management
QA Review: PR #4725Impact: gateway-service (CRITICAL) + apiml filter chain (CRITICAL) Acceptance Criteria Verification (Issue #4705)
All 6 acceptance criteria: VERIFIED ✅ Files Changed (5 files, +154/-7)
Test Adequacy✅ Adequate
CI Status44/44 checks PASS — all green (BuildAndTest, CITests, CITestsHA variants, SonarCloud, DCO, E2EUI, etc.) Timing Fix VerificationThe old code ran Pavel's Lens (8 Rules)
Minor Notes (non-blocking)
Verdict: ✅ QA PASSEDAll acceptance criteria verified. All CI checks green. No blocking issues. Pavel's Lens: all 8 rules checked, no issues. Recommendation: Ready to merge. |
QA Review — PR #4725 for #4705Reviewer: QA (Hermes pipeline) Acceptance Criteria Verification
Test Coverage AnalysisNew Tests (5):
Existing Tests Preserved:
Code QualityPositive:
SonarCloud:
Documentation Review
Risk AssessmentLow Risk:
VerdictPR #4725 fully implements issue #4705 acceptance criteria. Implementation is clean, well-tested, and backward compatible. All CI checks pass. Ready for merge. Recommendation: ✅ APPROVE and MERGE QA review by Hermes pipeline — task t_a6343f82 |
balhar-jakub
left a comment
balhar-jakub
left a comment
There was a problem hiding this comment.
Multi-Reviewer PR Review — PR #4725
Reviewers applied: 12 of 26 (pavel, achmelo, CarsonCook, janan07, ilkinabdullayev, plavjanik, weinfurt, pablocarle, taban03, arxioly, nxhafa, vsev0lod)
Inline comments: 13 | PR-level: 3
Per-reviewer summary
- pavel: Two duplicate attributes (
service.response_codevshttp.response.status_code);responseCodeonly set onON_COMPLETE— bug for error paths; magic path-element index 1; missing edge-case tests - nxhafa: Two hardcoded
doOnErrorhandlers should use constants; README PromQL is incorrect (dots not escaped) - ilkinabdullayev: Naming asymmetry between
responseCodeandstatusCode; string-concat on hot path - taban03:
doFinallywith conditional inside is hard to read — split intodoOnSuccess+doFinally - CarsonCook: Missing test for
ServiceNotFoundExceptionvsNoResourceFoundException - plavjanik: Test path assumption may not match gateway routing; backward-compat for
responseCode; missing 3rddoOnErrorhandler - janan07: README is excellent but missing operator-facing dashboard setup instructions
- pablocarle, arxioly, weinfurt, achmelo, vsev0lod: No relevant comments — paths didn't match their focus areas
PR-level comments
-
Missing test cases for
OtelRequestFilterTest.java: WhenattemptedServiceis at a different path position (e.g.,/or/gateway/api/v1/...), trailing slashes, malformed URIs, and nulle.getMessage()(theObjects.toStringfallback path). These edge cases will hit in production. (pavel) -
Backward-compat note for
OtelRequestContext: existingresponseCode(int)is preserved. But the newstatusCode(int)has identical behavior — just different storage key. This is a smell. Either rename the existing one and migrate, or document why both are needed. (plavjanik) -
Exception handling for
ServiceNotFoundException: PR #4705 mentions unknown service IDs, but the actual exception thrown by the routing chain may be different. Should we add a 3rddoOnErrorforServiceNotFoundException? (plavjanik)
Top actionable findings
- Bug:
responseCodeis only set onON_COMPLETE— error paths now setstatusCodebut the existingresponseCodeattribute is stale - Tests: 3 new tests should be parametrized; missing
ServiceNotFoundExceptiontest; missing edge cases - Docs: PromQL example has incorrect attribute names; README missing operator-facing dashboard setup
- Naming:
statusCodevsresponseCodeasymmetry is a smell - Constants: Error types and messages should be constants
|
|
||
| // capture attempted service for error messages | ||
| var pathElements = exchange.getRequest().getPath().elements(); | ||
| var attemptedService = pathElements.size() > 1 ? pathElements.get(1).value() : SERVICE_GATEWAY; |
There was a problem hiding this comment.
Why is path-element index 1 used to extract the service ID? This is the gateway filter — if we're inside APIML modulith, the path structure may differ (e.g., /gateway/api/v1/...). I would expect service ID extraction to be a shared helper, not a magic index. See how RoutingConfigurationErrorFilterFactory or EurekaUtils populate the service ID — there's likely something already there. (pavel)
|
|
||
| return filter.apply(exchange) | ||
| // downstream chain: route matching → routing → service call | ||
| .doOnError(NoResourceFoundException.class, e -> { |
There was a problem hiding this comment.
Two doOnError handlers with hardcoded status codes, error types, and error messages — these constants should be defined at the top of the class (or in OtelRequestContext) so they're discoverable and reusable. (nxhafa)
| .doOnError(NoResourceFoundException.class, e -> { | ||
| otelContext.statusCode(404); | ||
| otelContext.errorType("Service not onboarded"); | ||
| otelContext.errorMessage("Service " + attemptedService + " is not registered in the API ML"); |
There was a problem hiding this comment.
Error message is built by string concatenation on the hot path. Consider using a structured logger or at least a private constant for the prefix Service %s is not registered in the API ML. (ilkinabdullayev)
| .doOnError(ServiceNotAccessibleException.class, e -> { | ||
| otelContext.statusCode(503); | ||
| otelContext.errorType("Service instance not available"); | ||
| otelContext.errorMessage(Objects.toString(e.getMessage(), "No available instances")); |
There was a problem hiding this comment.
Objects.toString(e.getMessage(), "No available instances") — defaulting to a generic string when the exception message is null loses the operationally-useful context. Should we instead re-raise or log a warning? The current behavior silently swallows the missing message. (pavel)
| .map(HttpStatusCode::value) | ||
| .ifPresent(otelContext::responseCode); | ||
| } | ||
| otelContext.issue(); |
There was a problem hiding this comment.
The doFinally callback now mutates state conditionally on signalType == ON_COMPLETE. This couples the finally handler to the success path in a way that's easy to miss. Could be refactored to:
.doOnSuccess(t -> Optional.ofNullable(exchange.getResponse())
.map(ServerHttpResponse::getStatusCode)
.map(HttpStatusCode::value)
.ifPresent(otelContext::responseCode))
.doFinally(signalType -> otelContext.issue());This separates the success-handler and the issue-log-handler. (taban03)
| void givenUnknownService_whenNoResourceFound_thenSet404AndErrorType() { | ||
| var filter = new OtelRequestFilter(); | ||
|
|
||
| var request = MockServerHttpRequest.get("http://localhost/unknownservice/api/v1/data") |
There was a problem hiding this comment.
The test path /unknownservice/api/v1/data — the path element at index 1 is unknownservice. But the actual behavior on the gateway sidecar is that the first path element is the service ID. I see pathElements.get(1).value() is used as the fallback. Where is pathElements.get(0) assumed to be? Test paths should mirror production routing structure. (plavjanik)
| @Test | ||
| void givenUnknownService_whenNoResourceFound_thenSet404AndErrorType() { | ||
| var filter = new OtelRequestFilter(); | ||
|
|
There was a problem hiding this comment.
Missing test case: ServiceNotFoundException (when the service ID is known but not registered). The PR description mentions "unknown service ID" but the implementation only tests NoResourceFoundException. What's the distinction? (CarsonCook)
|
|
||
| @Test | ||
| void givenOtelContext_whenSetStatusCode_thenTransformToString() { | ||
| OtelRequestContext.of(exchange).statusCode(503); |
There was a problem hiding this comment.
Three new tests for statusCode, errorType, errorMessage — all follow the same pattern. Could be parametrized with the existing givenOtelContext_whenSetResponseCode_thenTransformToString test. (pavel)
| - `error.type` = `"Service instance not available"` | ||
| - `error.message` = the exception message from `ServiceNotAccessibleException` | ||
|
|
||
| ### Monitoring Use |
There was a problem hiding this comment.
The Prometheus alert examples use otel_log_count{error_type=...} — but the actual attribute name is error.type (with a dot). The PromQL syntax shown is incorrect. Should be error\.type or the attribute names should be normalized. (nxhafa)
2 participants
Closes #4705
Summary
Extend OtelRequestFilter to emit OpenTelemetry log signals when HTTP requests cannot be routed due to unknown Service ID or unavailable service instances. Fixes the response code timing bug.
Changes (3 areas, 3 commits)
Area A — OtelRequestContext attributes (gateway-service)
statusCode(int),errorType(String),errorMessage(String)http.response.status_code,error.type,error.messageresponseCode()→service.response_codepreserved for backward compatibilityArea B — OtelRequestFilter error handling (apiml)
.doOnError()hooks forNoResourceFoundException→ 404,error.type="Service not onboarded".doOnError()hooks forServiceNotAccessibleException→ 503,error.type="Service instance not available"doFinally(onON_COMPLETE) beforeissue(), fixing the bug whereissue()ran beforeresponseCode()was setStepVerifierArea C — Documentation (otel/README.md)
Build Validation
./gradlew clean build— BUILD SUCCESSFUL (410 tasks: 270 executed, 92 from cache, 48 up-to-date)(Architectural review to follow as a PR comment)