Implementation for the ldap connect server metric. by ruchikajha95 · Pull Request #50916 · keycloak/keycloak

ruchikajha95 · 2026-07-15T11:38:15Z

This PR adds a implementation for a metric that tracks LDAP connection attempts.

In LDAPOperationManager.execute() — the method every LDAP operation (search, create, modify, delete, etc.) flows through before talking to the LDAP server.

RESULTS:

Closes #50840

Signed-off-by: Ruchika ruchika.jha1@ibm.com

Copilot

Pull request overview

Adds LDAP connection-attempt timing and outcome metrics.

Changes:

Records connect duration and success/error outcome before LDAP operations.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

            tracing.error(e);
            throw e;
        } finally {
+            recordLdapRequest(success, startTimeNanos);


keycloak-github-bot

Unreported flaky test detected, please review

keycloak-github-bot · 2026-07-16T13:50:21Z

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.forms.BruteForceTest#testExceedMaxTemporaryLockouts

Keycloak CI - Base IT (5)

org.opentest4j.AssertionFailedError: Expected error event ==> expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
...

Report flaky test

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

+        requestTimer.withTags("operation", "connect", "outcome", success ? "success" : "error")
+                .record(durationNanos, TimeUnit.NANOSECONDS);


Deprecation is missing the since.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

            tracing.error(e);
            throw e;
        } finally {
+            recordLdapRequest(success, startTimeNanos);


Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

ahus1 · 2026-07-16T15:35:40Z

@ruchikajha95 - I just now realized that the test-method is actually not a test, but a test in the admin UI to verify that the connection works. This call doesn't include a metrics as of now, and therefore I wasn't able to deprecate the endpoint.

I'll merge it once the build is green.

I still wonder if we would eventually refactor it differently; for example having an LdapMetricProvider that has a MicrometerLdapMetricProvider. But that might be a future refactoring.

ruchikajha95 · 2026-07-16T15:45:39Z

@ruchikajha95 - I just now realized that the test-method is actually not a test, but a test in the admin UI to verify that the connection works. This call doesn't include a metrics as of now, and therefore I wasn't able to deprecate the endpoint.

I'll merge it once the build is green.

I still wonder if we would eventually refactor it differently; for example having an LdapMetricProvider that has a MicrometerLdapMetricProvider. But that might be a future refactoring.

Thanks for the review and for clarifying the test-method . Apologies for the confusion on my end. That makes sense now that it's the Admin UI connection test and not a unit test.
I agree that introducing an abstraction like LdapMetricProvider with a MicrometerLdapMetricProvider implementation would be a cleaner approach down the line.
Thanks again for the feedback and for merging it!

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

+        requestTimer.withTags("operation", "connect", "outcome", success ? "success" : "error")
+                .record(durationNanos, TimeUnit.NANOSECONDS);


Closes keycloak#50840 Signed-off-by: Ruchika <ruchika.jha1@ibm.com>

Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

ruchikajha95 self-assigned this Jul 15, 2026

ruchikajha95 requested a review from Copilot July 15, 2026 13:12

Copilot started reviewing on behalf of ruchikajha95 July 15, 2026 13:13 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

Comment thread ...ration/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPOperationManager.java Outdated

Comment thread ...ration/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPOperationManager.java Outdated

ruchikajha95 requested a review from Copilot July 16, 2026 07:51

Copilot started reviewing on behalf of ruchikajha95 July 16, 2026 07:51 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

Comment thread federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPContextManager.java

tracing.error(e);

throw e;

} finally {

recordLdapRequest(success, startTimeNanos);

keycloak-github-bot Bot added the flaky-test label Jul 16, 2026

keycloak-github-bot Bot reviewed Jul 16, 2026

View reviewed changes

ruchikajha95 marked this pull request as ready for review July 16, 2026 13:50

ruchikajha95 requested a review from a team as a code owner July 16, 2026 13:50

ruchikajha95 requested review from ahus1 and Copilot July 16, 2026 13:50

Copilot started reviewing on behalf of ruchikajha95 July 16, 2026 13:51 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

Comment thread federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPContextManager.java

Comment on lines +68 to +69

requestTimer.withTags("operation", "connect", "outcome", success ? "success" : "error")

.record(durationNanos, TimeUnit.NANOSECONDS);

ahus1 previously approved these changes Jul 16, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 16, 2026 14:33

Copilot started reviewing on behalf of ahus1 July 16, 2026 14:36 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

Comment thread federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPContextManager.java

tracing.error(e);

throw e;

} finally {

recordLdapRequest(success, startTimeNanos);

ahus1 self-assigned this Jul 16, 2026

Copilot AI review requested due to automatic review settings July 16, 2026 15:30

Copilot started reviewing on behalf of ahus1 July 16, 2026 15:31 View session

ahus1 force-pushed the feature-50840/Add-metric-for-connecting-to-the-LDAP-server- branch 2 times, most recently from defd196 to 83b64a6 Compare July 16, 2026 15:33

Copilot AI reviewed Jul 16, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings July 16, 2026 15:34

ahus1 enabled auto-merge (squash) July 16, 2026 15:35

ahus1 approved these changes Jul 16, 2026

View reviewed changes

Copilot started reviewing on behalf of ruchikajha95 July 16, 2026 15:43 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

Comment thread federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPContextManager.java

Comment on lines +73 to +74

requestTimer.withTags("operation", "connect", "outcome", success ? "success" : "error")

.record(durationNanos, TimeUnit.NANOSECONDS);

ruchikajha95 and others added 5 commits July 16, 2026 17:58

Implementation for the ldap connect server metric.

e45f273

Closes keycloak#50840 Signed-off-by: Ruchika <ruchika.jha1@ibm.com>

Implementation for the ldap connect server metric.

c320834

Closes keycloak#50840 Signed-off-by: Ruchika <ruchika.jha1@ibm.com>

Made the constructor and create method deprecated

c763192

Closes keycloak#50840 Signed-off-by: Ruchika <ruchika.jha1@ibm.com>

Review

89a0505

Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>

Review

3db9f20

Signed-off-by: Alexander Schwartz <alexander.schwartz@ibm.com>

Copilot AI review requested due to automatic review settings July 16, 2026 15:58

ahus1 force-pushed the feature-50840/Add-metric-for-connecting-to-the-LDAP-server- branch from 83b64a6 to 3db9f20 Compare July 16, 2026 15:58

Copilot started reviewing on behalf of ahus1 July 16, 2026 16:07 View session

Copilot AI reviewed Jul 16, 2026

View reviewed changes

ahus1 merged commit c1c17a8 into keycloak:main Jul 16, 2026
91 checks passed

		requestTimer.withTags("operation", "connect", "outcome", success ? "success" : "error")
		.record(durationNanos, TimeUnit.NANOSECONDS);

Uh oh!

Conversation

ruchikajha95 commented Jul 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

keycloak-github-bot Bot left a comment

Choose a reason for hiding this comment

Uh oh!

keycloak-github-bot Bot commented Jul 16, 2026

Unreported flaky test detected

org.keycloak.testsuite.forms.BruteForceTest#testExceedMaxTemporaryLockouts

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

ahus1 commented Jul 16, 2026

Uh oh!

ruchikajha95 commented Jul 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants