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

Using Keycloak's new built-in OTEL tracing #921

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

ahus1
Copy link
Contributor

@ahus1 ahus1 commented Aug 9, 2024

This simplifies our setup and we won't need EFS any more.

It will be a preview feature in KC26, and we will be able to add more items to it step-by-step.

Successful run in personal repo: https://github.com/ahus1/keycloak-benchmark/actions/runs/10356482913/job/28666434708

@ahus1 ahus1 self-assigned this Aug 9, 2024
@ahus1 ahus1 force-pushed the pr-enable-keycloak-otel-tracing branch 2 times, most recently from 850598b to 9b9ecab Compare August 12, 2024 17:18
@ahus1 ahus1 marked this pull request as ready for review August 13, 2024 10:29
Copy link
Contributor

@mhajas mhajas left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @ahus1! This simplifies things a lot!

I have two questions:

  1. The dashboards for auth code and client credentials grant are relying on OTEL, are the dashboards still working the same way as before? Do we need to upgrade some metric names?
  2. Otel is now disabled by default and as far as I know we don't enable it fo nightly run, should we enable it now since the setup is much simpler? https://github.com/ahus1/keycloak-benchmark/blob/9b9ecabc622eea908d508a21fe84e335d3983f63/provision/common/Taskfile.yaml#L5

@ahus1
Copy link
Contributor Author

ahus1 commented Aug 15, 2024

The dashboards for auth code and client credentials grant are relying on OTEL, are the dashboards still working the same way as before? Do we need to upgrade some metric names?

Thank you for raising that question. They used to rely on OTEL. In Keyloak 25 (?) we were able to finally enable Keycloak's native HTTP metrics including histograms, and now they work independently of OTEL. In fact, they are now available for any nightly run. Talking about that, I can now revert part of the 4992a27 change as REDIRECT is no longer used. I'll push a change.

Should we enable it now since the setup is much simpler?

I'd love to. Still I'd like to have this as a follow-up issue once this is agreed upon: I'd like to do a side-by-side analysis how much performance impact enabling OTEL has. On our last run we tracked down 3 ms regression, so it seems we are are spotting even small regressions here.

Does this answer your questions, and it is ok to merge?

Copy link
Contributor

@mhajas mhajas left a comment

Choose a reason for hiding this comment

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

Yes, this answers my questions. Thank you.

I added one more comment related to docker.io rate limits.

provision/rosa-cross-dc/Taskfile.yaml Show resolved Hide resolved
Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
@ahus1 ahus1 force-pushed the pr-enable-keycloak-otel-tracing branch from 9b9ecab to 61c4323 Compare August 15, 2024 13:42
@ahus1 ahus1 merged commit 8490d34 into keycloak:main Aug 15, 2024
3 checks passed
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.

2 participants