8000 Properly enable/disable metrics and health endpoints by pedroigor · Pull Request #11507 · keycloak/keycloak · GitHub
[go: up one dir, main page]

Skip to content

Properly enable/disable metrics and health endpoints - #11507

Merged
abstractj merged 1 commit into
keycloak:mainfrom
pedroigor:issue-11506
Jul 22, 2022
Merged

abstractj merged 1 commit into
keycloak:mainfrom
pedroigor:issue-11506

Conversation

@pedroigor
@pedroigor pedroigor commented Apr 19, 2022 •
Copy link
Copy Markdown
Contributor

Closes #11506

  • Completly remove the health and metrics endpoints when they are disabled
  • Fixes the health/live and health/ready endpoints to only show their specific checks
  • Avoid executing health and metrics endpoint in a worker thread as they run in their own worker thread, and avoid creating a KeycloakSession every time those endpoints are invoked. The current behavior is causing transactions to not disassociate from the calling thread and causing Arjuna to wait for them to timeout. Eventually, the server might block during the shutdown.
  • Fixes the KeycloakSecurityHeadersFilter in case the health and metrics endpoints are invoked where there is no session from the calling thread
  • Update docs with the current status of our health endpoint

There are many improvements to health and metrics we can do. This PR basically brings some consistency to what we have and fixes some known issues.

For health, we should be improving it as per #11556.

@pedroigor
pedroigor force-pushed the issue-11506 branch 7 times, most recently from dfa8497 to 6c8b8f9 Compare May 3, 2022 20:30
@abstractj
Copy link
Copy Markdown
Contributor

cc @keycloak/kc-developers

@pedroigor
pedroigor force-pushed the issue-11506 branch 2 times, most recently from 9419f20 to 99efc96 Compare June 9, 2022 15:12
@pedroigor
pedroigor force-pushed the issue-11506 branch 4 times, most recently from e78ba3e to 1fea26a Compare July 19, 2022 12:23
@pedroigor
pedroigor marked this pull request as ready for review July 19, 2022 13:16
@pedroigor
pedroigor force-pushed the issue-11506 branch 4 times, most recently from 9c97087 to aee2d5e Compare July 19, 2022 20:31
@pedroigor
pedroigor requested a review from DGuhr July 20, 2022 18:55
@DGuhr DGuhr left a comment •
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! A few things I wanted to ask about this one to understand the changes better, a typofix, but in general LGTM :)

Comment thread docs/guides/src/main/server/health.adoc Resolved
Comment thread docs/guides/src/main/server/health.adoc Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, just thinking now: should we still promote the build so much, or use start instead with the new behaviour around optimized? wdyt? not hard on either, though

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had the same thought and I initially started by using start (now we have pre-build by default). But I think we need to first see what it takes to update all guides and not a single one, in favor of consistency.

DGuhr
DGuhr previously approved these changes Jul 21, 2022
@DGuhr DGuhr left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks :)

abstractj
abstractj previously approved these changes Jul 21, 2022
@pedroigor
pedroigor dismissed stale reviews from abstractj and DGuhr via c071471 July 21, 2022 16:40
@pedroigor
pedroigor force-pushed the issue-11506 branch 2 times, most recently from c071471 to c444958 Compare July 21, 2022 16:41
Closes keycloak#11506

Co-authored-by: Dominik Guhr <dguhr@redhat.com>
@DGuhr DGuhr left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Properly enable/disable metrics and health endpoints

3 participants

0