Provide missing user event metrics from aerogear/keycloak-metrics-spi with a key… by bohmber · Pull Request #33046 · keycloak/keycloak

bohmber · 2024-09-18T08:58:50Z

…cloak mircometer event listener

inspired by
https://github.com/aerogear/keycloak-metrics-spi
https://github.com/please-openit/keycloak-native-metrics https://github.com/thomasdarimont/keycloak-project-example/tree/main/keycloak/extensions/src/main/java/com/github/thomasdarimont/keycloak/custom/metrics

Preview of the docs:
https://github.com/bohmber/keycloak/blob/issue-33043/docs/guides/server/event-metrics.adoc

fixes: #33043

keycloak-github-bot · 2024-09-18T10:03:46Z

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.webauthn.registration.passwordless.PwdLessResidentKeyRegTest#residentKeyNotRequiredPresent

Keycloak CI - WebAuthn IT (chrome)

java.lang.AssertionError: Expected RegisterPage but was Sign in to test (https://localhost:8543/auth/realms/test/protocol/openid-connect/auth?response_type=code&client_id=test-app&redirect_uri=https%3A%2F%2Flocalhost%3A8543%2Fauth%2Frealms%2Fmaster%2Fapp%2Fauth&state=f892fe75-87a3-4861-8834-4585a812b5db&scope=openid)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.pages.AbstractPage.assertCurrent(AbstractPage.java:47)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
...

Report flaky test

keycloak-github-bot

Unreported flaky test detected, please review

bohmber · 2024-09-19T08:29:11Z

TODOs:
Metrics names prefix vendor.kc or vendor or keycloak or vendor.keycloak? @ahus1 suggested the prefix keycloak.
Which metrics should be part of the basic configuration?

keycloak-github-bot

Unreported flaky test detected, please review

keycloak-github-bot · 2024-09-19T10:11:23Z

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.webauthn.registration.passwordless.PwdLessOtherSettingsTest#defaultValues

Keycloak CI - WebAuthn IT (chrome)

java.lang.AssertionError: Expected RegisterPage but was Sign in to test (https://localhost:8543/auth/realms/test/protocol/openid-connect/auth?response_type=code&client_id=test-app&redirect_uri=https%3A%2F%2Flocalhost%3A8543%2Fauth%2Frealms%2Fmaster%2Fapp%2Fauth&state=3ef29f94-72df-44b9-94e0-d7bcc7e568f4&scope=openid)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.pages.AbstractPage.assertCurrent(AbstractPage.java:47)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
...

Report flaky test

org.keycloak.testsuite.webauthn.registration.passwordless.PwdLessPolicyJsInjectionTest#relyingPartyEntityName

Keycloak CI - WebAuthn IT (chrome)

java.lang.AssertionError: Expected RegisterPage but was Sign in to test (https://localhost:8543/auth/realms/test/protocol/openid-connect/auth?response_type=code&client_id=test-app&redirect_uri=https%3A%2F%2Flocalhost%3A8543%2Fauth%2Frealms%2Fmaster%2Fapp%2Fauth&state=63187ca7-8d4c-4901-ab1b-f31b287b839d&scope=openid)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.pages.AbstractPage.assertCurrent(AbstractPage.java:47)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
...

Report flaky test

ahus1 · 2024-09-20T15:44:04Z

@bohmber - thank you for this PR. Please provide an example output of the metrics you created.

For the prefix, I'd go with keycloak.

ahus1 · 2024-09-20T15:50:22Z

@bohmber - another note: We would need some docs for this similar to what the Aerogear extension provides. If you re-use documentation or code from that repo, you need to attribute the changes to the original authors. For now, use the co-author in the commits. I might need to figure out how to do this properly to comply with our DCO.

bohmber · 2024-09-20T16:39:17Z

@ahus1 documentation is a good topic. I need to figure out what makes sense. For the legal stuff I think I have to recreate the pr/commits with all co-authors

keycloak-github-bot · 2024-09-21T08:55:42Z

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.webauthn.registration.passwordless.PwdLessPolicyJsInjectionTest#relyingPartyEntityName

Keycloak CI - WebAuthn IT (chrome)

java.lang.AssertionError: Expected RegisterPage but was Sign in to test (https://localhost:8543/auth/realms/test/protocol/openid-connect/auth?response_type=code&client_id=test-app&redirect_uri=https%3A%2F%2Flocalhost%3A8543%2Fauth%2Frealms%2Fmaster%2Fapp%2Fauth&state=6f2f477b-1cde-4b31-b6f8-306d544f70d6&scope=openid)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.pages.AbstractPage.assertCurrent(AbstractPage.java:47)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
...

Report flaky test

org.keycloak.testsuite.webauthn.registration.passwordless.PwdLessResidentKeyRegTest#residentKeyNotRequiredPresent

Keycloak CI - WebAuthn IT (chrome)

java.lang.AssertionError: Expected RegisterPage but was Sign in to test (https://localhost:8543/auth/realms/test/protocol/openid-connect/auth?response_type=code&client_id=test-app&redirect_uri=https%3A%2F%2Flocalhost%3A8543%2Fauth%2Frealms%2Fmaster%2Fapp%2Fauth&state=b0687469-e672-4db0-b1e5-c654102f8cf8&scope=openid)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.pages.AbstractPage.assertCurrent(AbstractPage.java:47)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
...

Report flaky test

keycloak-github-bot

Unreported flaky test detected, please review

ahus1 · 2024-09-23T06:07:03Z

@bohmber - I see you rebased or merged the main branch into the PR. Please avoid this unless you see conflicts or are being asked by a maintainer to do so as it avoid unnecessary rebuilds of the branch and notifications to the maintainers. Thanks!

bohmber · 2024-09-23T06:25:47Z

@ahus1 That is fine for me. I added some basic documentation and you can disable AdminEvents and Events and the metrics enabled will be checked by the factory

ahus1 · 2024-10-18T19:08:10Z

Thank you @vmuzikar for the review. I contributed the CLI options so I will comment on that part next week.

keycloak-github-bot

Unreported flaky test detected, please review

keycloak-github-bot · 2024-10-28T22:17:20Z

Unreported flaky test detected

If the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.broker.KcOidcBrokerTest#testPostBrokerLoginFlowWithOTP_bruteForceEnabled

Keycloak CI - Java Distribution IT (windows-latest - temurin - 17)

java.lang.AssertionError: expected:<3> but was:<2>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.junit.Assert.assertEquals(Assert.java:633)
...

Report flaky test

ahus1 · 2024-10-29T08:58:43Z

@vmuzikar - tests are green, ready for another round of review when you have the time. Thanks!

mhajas · 2024-10-29T08:59:38Z

We should disable the feature after the test to make sure the feature does not affect the tests running afterwards.

OK, I've reworked the test and used the same logic as in HostnameV2Test.java. Please re-review.

mhajas · 2024-10-29T13:09:36Z

+        if (suiteContext.getAuthServerInfo().isQuarkus()) {
+            AbstractQuarkusDeployableContainer container = (AbstractQuarkusDeployableContainer) suiteContext.getAuthServerInfo().getArquillianContainer().getDeployableContainer();
+            List<String> args = new ArrayList<>();
+            args.add("--features=user-event-metrics");


I just realized this won't work if an additional feature is enabled for the test using auth.server.feature. I think we should use the "official" way for enabling features. It means one more restart in the test, however, we will be sure the feature set after the test is the same as before.

Here is the commit: mhajas@1000c93 let me know if you agree with the change and I can push directly to this PR.

Thank you for spotting this. I've added it to the PR, squashed it, and rebased it as it had some conflicts.

… to a keycloak micrometer event listener inspired by https://github.com/aerogear/keycloak-metrics-spi https://github.com/please-openit/keycloak-native-metrics Closes keycloak#33043 Signed-off-by: Bernd Bohmann <bommel@apache.org> Signed-off-by: Alexander Schwartz <aschwart@redhat.com> Signed-off-by: Michal Hajas <mhajas@redhat.com> Co-authored-by: Alexander Schwartz <aschwart@redhat.com> Co-authored-by: Michal Hajas <mhajas@redhat.com> Signed-off-by: Michal Hajas <mhajas@redhat.com>

mhajas

Thanks for the great contribution @bohmber @ahus1.

I removed an unused import and I believe this is good to be merged.

We should probably wait for reviews from other people though.

addressed

vmuzikar

@ahus1 Sorry for the late reply and thank you for the updates! The latest changes look good.

However, I noticed one last config/UX issue. It's possible to specify global event listener as a realm listener. This should have no effect but still could be confusing to the user:

At least I'd say we should filter out the global listeners from the UI. But we could take it step further if we want: throw an exception if a global listener is added to a realm (to cover the REST API interface). And ideally as a follow-up maybe display the global listener as a permanent/static/grayed out one in the list of realm listeners in the UI.

vmuzikar · 2024-11-01T18:34:20Z

+            setConfigProperty("keycloak.metrics-enabled", "true");
+            setConfigProperty("keycloak.event-metrics-user-enabled", "true");


I believe this won't work without proper modifications of the Undertow config file.

Also it's missing the optional settings for tags and events.

The question is: do we care? Undertow is no longer used in any of ours CIs, AFAIK. It's used only for dev purposes. We could simply say "this test runs only on Quarkus".

The question is: do we care? Undertow is basically used as a dev tool, AFAIK it's not used in any CI any more. We could simply say that the test supports Quarkus only.

vmuzikar · 2024-11-01T18:50:06Z

That said, we could IMHO resolve the UX as follow up not to hold this PR any longer.

vmuzikar

In the light of my previous comment, approving as is. We can polish in follow-ups.

ahus1 · 2024-11-04T07:56:02Z

Created follow-up-issue #34602

graziang · 2024-11-04T09:30:26Z

@ahus1 Is it possible that this PR is causing errors in the "Store Model Tests" with jpa profile for some tests in AdminEventQueryTest?

ahus1 · 2024-11-04T09:56:29Z

@graziang - I see that the CI test suite skipped them as it didn't consider them related, so it could be. Do you have a pointer with a failed GHA?

ahus1 · 2024-11-04T10:11:08Z

Issue created: #34611

bohmber requested review from a team as code owners September 18, 2024 08:58

keycloak-github-bot Bot added team/cloud-native flaky-test labels Sep 18, 2024

keycloak-github-bot Bot mentioned this pull request Sep 18, 2024

Flaky test: org.keycloak.testsuite.oidc.AuthenticationMethodReferenceTest#testAmrPastMaxAge #26117

Closed

keycloak-github-bot Bot reviewed Sep 18, 2024

View reviewed changes

bohmber force-pushed the issue-33043 branch from 7337ac8 to 1a74c94 Compare September 19, 2024 08:25

bohmber marked this pull request as draft September 19, 2024 08:26

keycloak-github-bot Bot reviewed Sep 19, 2024

View reviewed changes

bohmber force-pushed the issue-33043 branch from 238b3dd to 3a7c619 Compare September 20, 2024 16:48

keycloak-github-bot Bot reviewed Sep 21, 2024

View reviewed changes

vmuzikar self-requested a review September 23, 2024 07:02

This was referenced Sep 23, 2024

Flaky test: org.keycloak.testsuite.webauthn.WebAuthnIdlessTest#testWebAuthnIDLessAndWebAuthnAndWebAuthnPasswordlessLogin #32473

Closed

Flaky test: org.keycloak.testsuite.webauthn.WebAuthnRegisterAndLoginTest#registerUserSuccess #32721

Closed

ahus1 requested review from a team as code owners October 28, 2024 21:07

keycloak-github-bot Bot added team/core-clients team/core-iam labels Oct 28, 2024

keycloak-github-bot Bot reviewed Oct 28, 2024

View reviewed changes

ahus1 requested a review from vmuzikar October 29, 2024 08:58

mhajas suggested changes Oct 29, 2024

View reviewed changes

ahus1 requested a review from mhajas October 29, 2024 09:22

mhajas reviewed Oct 29, 2024

View reviewed changes

ahus1 force-pushed the issue-33043 branch from 25a706d to 95fad49 Compare October 29, 2024 13:18

ahus1 requested a review from mhajas October 29, 2024 13:18

mhajas force-pushed the issue-33043 branch from 95fad49 to ea5e623 Compare October 29, 2024 13:24

mhajas approved these changes Oct 29, 2024

View reviewed changes

vmuzikar requested changes Nov 1, 2024

View reviewed changes

vmuzikar approved these changes Nov 1, 2024

View reviewed changes

ahus1 mentioned this pull request Nov 4, 2024

Rework global event listener for metrics #34602

Closed

1 of 2 tasks

ahus1 merged commit 7681687 into keycloak:main Nov 4, 2024

ahus1 mentioned this pull request Nov 4, 2024

AdminEventQueryTest test fails after adding global event listener #34611

Closed

2 tasks done

		setConfigProperty("keycloak.metrics-enabled", "true");
		setConfigProperty("keycloak.event-metrics-user-enabled", "true");

Conversation

Uh oh!

Uh oh!

Unreported flaky test detected

org.keycloak.testsuite.webauthn.registration.passwordless.PwdLessResidentKeyRegTest#residentKeyNotRequiredPresent

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Unreported flaky test detected

org.keycloak.testsuite.webauthn.registration.passwordless.PwdLessOtherSettingsTest#defaultValues

org.keycloak.testsuite.webauthn.registration.passwordless.PwdLessPolicyJsInjectionTest#relyingPartyEntityName

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Unreported flaky test detected

org.keycloak.testsuite.webauthn.registration.passwordless.PwdLessPolicyJsInjectionTest#relyingPartyEntityName

org.keycloak.testsuite.webauthn.registration.passwordless.PwdLessResidentKeyRegTest#residentKeyNotRequiredPresent

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Unreported flaky test detected

org.keycloak.testsuite.broker.KcOidcBrokerTest#testPostBrokerLoginFlowWithOTP_bruteForceEnabled

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants