oscerd commented on PR #1929:
URL:
https://github.com/apache/camel-spring-boot/pull/1929#issuecomment-5510231576
Hi @Croway — I looked at this because I had implemented the same change a
week ago (as part of CAMEL-24499) and then reverted it in review. Most of your
rationale holds up, but one load-bearing part of it does not, and I think it's
worth settling before this merges.
**The camel-main point is correct.** `ManagementHttpServer` does gate it:
`ctx.request().getParam("stackTrace")` at line 641, `error-stacktrace` emitted
at line 976. Opt-in there, as you say.
**The microprofile point is not.** On current camel `main`,
`CamelMicroProfileHealthHelper.applyHealthDetail` is structurally identical to
what `CamelHealthHelper` looks like before this PR:
```java
result.getError().ifPresent(error -> {
builder.withData("error.message", error.getMessage());
final String s = ExceptionHelper.stackTraceToString(error);
builder.withData("error.stacktrace", s);
});
```
The error block sits outside the `exposureLevel.equals("full")` branch,
exactly as it did here — so camel-microprofile-health **does** emit
`error.stacktrace` at the `default` level.
`CamelMicroProfileHealthCheckTest.testExposureLevelDefault` asserts it:
```java
assertEquals("Forced exception", result.getString("error.message"));
assertNotNull(result.getString("error.stacktrace"));
```
I think the confusion is `camel-quarkus-microprofile-health` vs
`camel-microprofile-health`. The former is in the camel-quarkus repo and may
well behave as you describe; the latter is the one in this repo, and it is the
one CAMEL-18832 aligned to — the commit message says "Aligned output to be
similar to microprofile-health", and the original implementation already placed
the error block outside the branch. So the placement here was deliberate, not
accidental.
That doesn't make the change wrong. My reading is that the *current*
behaviour is too verbose for a default and your instinct is right — but as
written this PR diverges camel-spring-boot from camel-microprofile-health
rather than matching it, which is the opposite of what the description claims,
and it leaves a passing test in the other runtime asserting the behaviour
you're removing here.
Two ways I can see to resolve it:
1. **Change both runtimes together** — gate `error.stacktrace` on `full` in
`CamelMicroProfileHealthHelper` as well and update `testExposureLevelDefault`.
That delivers what the description says and keeps the alignment CAMEL-18832
established.
2. **Accept the divergence deliberately** — merge this as-is but drop the
microprofile justification from the description, so the guide and the commit
history record it as an intentional Spring Boot-only choice rather than an
alignment.
Either is fine by me; I just don't think it should land on the current
wording. Happy to do the microprofile half if option 1 is the direction — the
change there is the same three lines.
For what it's worth, I also think the bigger exposure problem is CAMEL-24498
(your #1927): at Spring Boot's own default of `show-details=never` none of this
reaches an unauthenticated caller at all, and it's the observability starter
forcing `exposure-level=full` and `show-details=always` that makes it visible.
cc @davsclaus
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
