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

Keycloak: Add opentelemetry tracing #40

Merged
merged 4 commits into from
Nov 21, 2022

Conversation

sonOfRa
Copy link
Contributor

@sonOfRa sonOfRa commented Nov 11, 2022

No description provided.

@sonOfRa sonOfRa marked this pull request as draft November 11, 2022 10:29
@thomasdarimont
Copy link
Owner

Great work!

How about adding a section to the readme with a small description of the setup and how to demo?

Clickable links would also be great! I think we have a dedicated ops domain like ops.acme.test intended for such use cases.

@sonOfRa
Copy link
Contributor Author

sonOfRa commented Nov 12, 2022

Yeah, docs are still needed, especially on how to turn on tracing for some of the apps as well. Another missing detail is the support of TLS for both the Jaeger UI as well as the collection endpoints.

@thomasdarimont
Copy link
Owner

Is it possible to exclude traces for certain paths, e.g. auth/resources/...?
image

@sonOfRa
Copy link
Contributor Author

sonOfRa commented Nov 12, 2022

I'll have to investigate, a quick search didn't yield a lot of results. Something I'd also like is for certain "Operations" to be grouped together. Currently, two requests to the openid-connect/auth endpoint are treated as different "operations" by jaeger because the GET parameters are different. There have been some changes to the otel-extension in newer Quarkus version, so a test with Keycloak 20 and Quarkus 2.13.3.Final (KC 19 uses 2.7.6.Final) might yield better results in general. The newer versions definitely come with MDC Injection, which the 2.7.6 Version still lacks, so there are no spanIds in logs for now.

@thomasdarimont
Copy link
Owner

Perhaps this dicussion contains some hints about ignoring certain urls: open-telemetry/opentelemetry-java-instrumentation#1060

@thomasdarimont
Copy link
Owner

thomasdarimont commented Nov 12, 2022

How about the following:

Then we have a more options for the showcase :)

Note that after the Keycloak 20.0.1 upgrade, you need to start with a fresh h2 database - otherwise Keycloak 20.x will fail to start... There seem to be options to migrate an older h2 database to a newer one, but I did not try it.

@sonOfRa
Copy link
Contributor Author

sonOfRa commented Nov 12, 2022

There's also https://quarkus.io/guides/opentelemetry#quarkus-opentelemetry_quarkus.opentelemetry.tracer.suppress-non-application-uris, which excludes quarkus /q endpoints by default. Maybe we can extend the extension here to allow additional exclude-urls based on path matching.

@thomasdarimont
Copy link
Owner

thomasdarimont commented Nov 12, 2022

@sonOfRa okay, I'm going to merge Keycloak 20.0.x into main now.

Done via 559f075

@sonOfRa sonOfRa force-pushed the opentelemetry branch 2 times, most recently from 6713e17 to 1f41d54 Compare November 16, 2022 09:53
@sonOfRa
Copy link
Contributor Author

sonOfRa commented Nov 16, 2022

Looks quite a bit better with KC 20, we now have MDC injection for logging, in both JSON and plain:

trace-console-log
trace-json-log

Setting the service name also works, and the "operations" are grouped by HTTP Request Method, rather than HTTP Request Method + Full URL.

jaeger-operations

Excluding /resources URLs does not seem to be working, despite that being on by default. My guess here is that Quarkus does not recognize Kecloak resource URLs as resource URLs, and so they are not ignored. This might need some extra work in the quarkus extension to allow manual exclusion of certain URLs.

The http.target tag still includes all Query Parameters, like:
/auth/realms/master/protocol/openid-connect/auth?client_id=security-admin-console&redirect_uri=https%3A%2F%2Fid.acme.test%3A8443%2Fauth%2Fadmin%2Fmaster%2Fconsole%2F&state=0733bbd4-6c4f-40ed-a084-7b2fb5735698&response_mode=fragment&response_type=code&scope=openid&nonce=122abb8f-f06c-4052-8ae1-18f5c2d4588c&code_challenge=9hV1mdvqzXBmFOa2vlJSdqF3A5gZKLl6rLTiUxT-VbE&code_challenge_method=S256, which makes it impossible to do a search for something like "Show me all traces involving the OIDC authorization_endpoint", since Jaeger only allows exact matches on tags. This might also need some extra work on the extension, maybe by introducing a tag that only includes the URL, or dropping query parameters from the target tag.

I'll try to get TLS working for everything, and then this should be ready to merge. The exclusions and better tagging are probably something to treat as future improvements for this setup.

@sonOfRa sonOfRa marked this pull request as ready for review November 18, 2022 10:43
@sonOfRa
Copy link
Contributor Author

sonOfRa commented Nov 18, 2022

I would say this PR is complete. I opened #42 and #43 for potential future improvements to the telemetry setup.

@thomasdarimont
Copy link
Owner

LGTM

@thomasdarimont thomasdarimont merged commit c348c7a into thomasdarimont:main Nov 21, 2022
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