Log an error if health indicator throws an exception #9110
Conversation
spring-projects-issues
added
the
status: waiting-for-triage
| connection.getMetaData().getJMSProviderName()); | ||
| } | ||
| catch (Exception ex) { | ||
| logger.warn(String.format( |
There was a problem hiding this comment.
I don't like that this is JMS-specific. Why not log something in AbstractHealthIndicator?
There was a problem hiding this comment.
Ditto. My initial reaction was "why do we need to log something here specifically?". We could add the exception to the detail as well.
There was a problem hiding this comment.
Makes sense, I'll add the information to the AbstractHealthIndicator.
@dsyer |
gytis
changed the title
|
Updated commit to write warning message in AbstractHealthIndicator |
| logger.warn(String.format( | ||
| "%s failed: %s", | ||
| this.getClass().getSimpleName(), | ||
| ex.getMessage())); |
There was a problem hiding this comment.
I think it'd be better to include the whole exception by using warn(String, Throwable) rather than just the message, particularly as the message is already part of the health's detail
| catch (Exception ex) { | ||
| logger.warn(String.format( | ||
| "%s failed: %s", | ||
| this.getClass().getSimpleName(), |
There was a problem hiding this comment.
To avoid ambiguity, I think it'd be better to use getName()
There was a problem hiding this comment.
Wouldn't it be better to have a non static logger and use getClass() as the source? That way the classname of the health indicator would show up in the log entry by default.
There was a problem hiding this comment.
It probably would, however I don't think we'd done that anywhere else in the codebase. If the logger's non-static we probably need to think about exposing it to subclasses.
There was a problem hiding this comment.
Any suggestion which option should I add? I see both static and non-static examples in the code base.
There was a problem hiding this comment.
Let's go for private final Log please. We can think about exposing it to subclasses separately if/when the need arises.
wilkinsona
added
the
status: waiting-for-feedback
|
@wilkinsona I've changed logger to non-static and added getClass() as log name to make message cleaner. |
wilkinsona
added
priority: normal
type: enhancement
* gh-9110: Log a warning if a health indicator throws an exception
|
Thanks for the PR. It's now been merged into 1.5.x and master. |
5 participants
When running an application on Kubernetes its probes use /health endpoint to check if application is ready and alive. In case if JMS connection is not available, probes fail and pod is killed and restarted. However, the error message is simply that 'readiness/liveliness probe has failed'. Because JMS health indicator doesn't print any logging about failure, it's hard to figure out why did the probe fail.