Include failing/timed out check name in health/readiness check response #638
Conversation
|
I like the idea and I don't see a reason for not improving the message. |
longshorej
force-pushed
the
health-name
branch
2 times, most recently
from
a0b17d9
to
528ad78
Compare
longshorej
changed the title
|
Cool, thanks @renatocaval. This is ready for a review. |
| @@ -33,14 +33,14 @@ abstract class HealthChecks { | |||
| /** | |||
| * Returns Future(true) if the system is ready to receive user traffic | |||
| */ | |||
| def ready(): Future[Boolean] | |||
| def ready(): Future[Either[String, Unit]] | |||
There was a problem hiding this comment.
This changes the public API in a non-compatible way.
We'd have to add new methods and delegate
There was a problem hiding this comment.
Okay, that should be simple enough, I'll update when I have some time.
| @@ -30,16 +29,16 @@ final class HealthChecks(system: ExtendedActorSystem, settings: HealthCheckSetti | |||
| /** | |||
| * Returns CompletionStage(true) if the system is ready to receive user traffic | |||
| */ | |||
| def ready(): CompletionStage[java.lang.Boolean] = | |||
| delegate.ready().map(Boolean.box)(ExecutionContexts.sameThreadExecutionContext).toJava | |||
| def ready(): CompletionStage[Either[String, Unit]] = | |||
There was a problem hiding this comment.
we shouldn't use Either in javadsl
There was a problem hiding this comment.
Good point - ideas on what a good alternative would be for Java (if this PR goes forward)? Maybe Optional<String> (plus delegated methods requested by @chbatey)?
There was a problem hiding this comment.
Probably a custom type is more idiomatic in Java
I'll look into it, but atm I'm not sure how unfortunately, as I think the majority of the utility is from having the messages directly in the HTTP response. Consider it may be a bit more difficult to obtain host logs than it is to hit an endpoint. |
|
I agree it will be very useful to have this in the error message so you can see it from the place doing the pinging of the health checks. If we can do it in an API compatible way I am +1 to doing this |
|
Okay, I've pushed an update to ensure the API remains compatible, let me know what y'all think. Thanks! |
This modifies the readiness/health check internals to ensure that when a check fails, for any reason, that the check's class name is included in the message to aid in debugging. Additionally, timeouts are now tracked on a per-check basis so that it's clear which check timed out.
| /** | ||
| * Returns CompletionStage(true) if the system is ready to receive user traffic | ||
| */ | ||
| def ready(): CompletionStage[java.lang.Boolean] = | ||
| delegate.ready().map(Boolean.box)(ExecutionContexts.sameThreadExecutionContext).toJava | ||
| readyResult().thenApply(((r: CheckResult) => r.isSuccess).asJava) |
There was a problem hiding this comment.
Originally had .thenApply(_.iSuccess).asJava but that failed to compile on 2.11:
[error] /Users/jlongshore/work/tubi/akka-management/management/src/main/scala/akka/management/javadsl/HealthChecks.scala:38:29: missing parameter type for expanded function ((x$2) => x$2.isSuccess)
[error] readyResult().thenApply(_.isSuccess)
[error] ^
[error] /Users/jlongshore/work/tubi/akka-management/management/src/main/scala/akka/management/javadsl/HealthChecks.scala:52:29: missing parameter type for expanded function ((x$4) => x$4.isSuccess)
[error] aliveResult().thenApply(_.isSuccess)
[error] ^
[error] two errors found
Include failing check name in health/readiness check messages/responses
This modifies the readiness/health check internals to ensure that when a check fails, for any reason, that the check's class name is included in the message to aid in debugging. Additionally, timeouts are now tracked on a per-check basis so that it's clear which check timed out.
I haven't updated the tests yet -- if I could get some validation/assurance that such a chance would be accepted, then I'd be happy to get the tests passing.Ready for a review.
Example failure: