Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make it possible to override logExceptionIfPresent in AbstractHealthIndicator and AbstractReactiveHealthIndicator #43492

Closed
marcusvoltolim opened this issue Dec 12, 2024 · 5 comments
Labels
status: declined A suggestion or change that we don't feel we should currently apply

Comments

@marcusvoltolim
Copy link

Rename and change the visibility of the logExceptionIfPresent method to protected so that it is possible to replace the default logic when necessary, increasing the possibility of reusing this abstract class.

@marcusvoltolim marcusvoltolim changed the title [spring-boot-actuator] Improving AbstractHealthIndicator to make possible overrides logExceptionIfPresent [spring-boot-actuator] Improving AbstractHealthIndicator and AbstractReactiveHealthIndicator to make possible overrides logExceptionIfPresent Dec 12, 2024
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 12, 2024
@philwebb
Copy link
Member

Can provide some more details about how/why you want to replace the default logic?

@philwebb philwebb added the status: waiting-for-feedback We need additional information before we can continue label Dec 12, 2024
@marcusvoltolim
Copy link
Author

marcusvoltolim commented Dec 12, 2024

Can provide some more details about how/why you want to replace the default logic?

Of course!
Features such as metrics, sending alerts in Slack, WebHook or simply changing the log level from warn to error.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Dec 12, 2024
@wilkinsona wilkinsona changed the title [spring-boot-actuator] Improving AbstractHealthIndicator and AbstractReactiveHealthIndicator to make possible overrides logExceptionIfPresent Make is possible to override logExceptionIfPresent in AbstractHealthIndicator and AbstractReactiveHealthIndicator Dec 13, 2024
@philwebb
Copy link
Member

I feel like we may need to improve the way that we log health indicator errors rather than change the visibility of logExceptionIfPresent. For the use-cases you've described, it feels odd that only certain AbstractReactiveHealthIndicator subclasses would get the modified behavior. I think we should move the logging code to somewhere central so that it can be tweaked.

I'm also not convinced that we should be sending alerts as a side effect of calling the health indicator. Currently the indicators are invoked only when someone hits the /actuator/health endpoint. Sending alerts feels like something that should happen based on scheduled checks. There are also complexities around sending alerts as you probably only want to send them when a health indicator result changes. Generally speaking, I think the system that is invoking /actuator/health should be the thing responsible for sending alerts.

I'd like to repurpose this issue for relocating the logging code and making it easier to change. I'm not convinced that we should be adding scheduled health indicator invocations just yet.

Let's see what the rest of the team thinks.

@philwebb philwebb added the for: team-attention An issue we'd like other members of the team to review label Dec 13, 2024
@wilkinsona wilkinsona changed the title Make is possible to override logExceptionIfPresent in AbstractHealthIndicator and AbstractReactiveHealthIndicator Make it possible to override logExceptionIfPresent in AbstractHealthIndicator and AbstractReactiveHealthIndicator Dec 16, 2024
@marcussatelis
Copy link

@philwebb
Copy link
Member

I've opened a new issue (#43589) to look at centralizing the logging.

@philwebb philwebb closed this as not planned Won't fix, can't repro, duplicate, stale Dec 20, 2024
@philwebb philwebb added status: declined A suggestion or change that we don't feel we should currently apply and removed for: team-attention An issue we'd like other members of the team to review status: waiting-for-triage An issue we've not yet triaged status: feedback-provided Feedback has been provided labels Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Development

No branches or pull requests

4 participants