Skip to content

Added the LDAP operation type and error name in the metric - #51491

Draft
ruchikajha95 wants to merge 4 commits into
keycloak:mainfrom
ruchikajha95:feature-50958/operation-type-and-error-name-to-LDAP
Draft

Added the LDAP operation type and error name in the metric#51491
ruchikajha95 wants to merge 4 commits into
keycloak:mainfrom
ruchikajha95:feature-50958/operation-type-and-error-name-to-LDAP

Conversation

@ruchikajha95

@ruchikajha95 ruchikajha95 commented Aug 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor
image

Closes #50958

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

@ruchikajha95
ruchikajha95 force-pushed the feature-50958/operation-type-and-error-name-to-LDAP branch 3 times, most recently from cd24bd9 to 1cf7c5a Compare August 6, 2026 20:13
@keycloak-github-bot

keycloak-github-bot Bot commented Aug 7, 2026

Copy link
Copy Markdown

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.actions.TermsAndConditionsTest#termsAccepted

Keycloak CI - Forms IT (firefox)

java.lang.RuntimeException: Could not create statement
	at org.jboss.arquillian.junit.Arquillian.methodBlock(Arquillian.java:307)
	at org.jboss.arquillian.junit.Arquillian$2.evaluate(Arquillian.java:160)
	at org.jboss.arquillian.junit.Arquillian.multiExecute(Arquillian.java:344)
	at org.jboss.arquillian.junit.Arquillian.access$200(Arquillian.java:48)
...

Report flaky test

keycloak-github-bot[bot]
keycloak-github-bot Bot reviewed Aug 7, 2026
View reviewed changes

@keycloak-github-bot keycloak-github-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@ruchikajha95 ruchikajha95 self-assigned this Aug 7, 2026
@ruchikajha95
ruchikajha95 requested a balanced review from Copilot August 7, 2026 08:59
Copilot AI reviewed Aug 7, 2026
View reviewed changes

Copilot AI 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.

Pull request overview

Adds operation and error labels to LDAP request metrics.

Changes:

  • Adds LDAP operation names to execute metrics.
  • Adds exception class names to failed request metrics.
  • Classifies supported LDAP operations.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
LDAPOperationManager.java Adds operation and error metric tags.
LDAPContextManager.java Adds error tags to connection metrics.
Suppressed comments (1)

federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPOperationManager.java:865

  • The linked issue requires changing the LDAP execute trace spans, but these values are only added to the timer. The span is still named LDAPOperationManager.execute, still stores the full multi-line operation.toString(), and receives no structured operation/error/DN attributes, so closing #50958 would leave all of its stated trace problems unresolved; update the span instrumentation or stop closing that issue.
            recordLdapRequest("execute", operation.operationType(), success, startTimeNanos, errorName);

Comment thread ...ration/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPOperationManager.java Outdated
Comment thread federation/ldap/src/main/java/org/keycloak/storage/ldap/idm/store/ldap/LDAPContextManager.java Outdated
@ruchikajha95
ruchikajha95 requested review from ahus1 and pruivo August 10, 2026 09:43
@pruivo

pruivo commented Aug 10, 2026

Copy link
Copy Markdown
Member

@ruchikajha95 do we need both operation and operation_name?

@ruchikajha95

ruchikajha95 commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

@ruchikajha95 do we need both operation and operation_name?

The idea was "operation" tag = "execute" or "authenticate" : this is the high-level category

  • "operation_name" tag = the specific LDAP operation type (e.g. "search", "create", "modify", "unknown") returned by operationType() so that the metrics can explain the type better.

@pruivo

pruivo commented Aug 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Can you list all possible values? if operation only has two values, it seems overkill to have 2 tags.

edit: I'm asking this because PromQL allows query aggregation, making the operation (the high category) tag irrelevant.

@ruchikajha95

ruchikajha95 commented Aug 10, 2026

Copy link
Copy Markdown
Contributor Author

Can you list all possible values? if operation only has two values, it seems overkill to have 2 tags.

The complete list of possible values are :

  • operation tag: only 2 values — "execute" and "authenticate"

    • operation_name tag: "remove", "rename", "search", "searchPaginated", "lookupById", "modify", "create", "passwordModify", "unknown" (default)

    I agree to your point . Do u suggest that to merge them into a single "operation" tag where "authenticate" is one value alongside the specific LDAP operation types

@pruivo

pruivo commented Aug 10, 2026

Copy link
Copy Markdown
Member

Can you list all possible values? if operation only has two values, it seems overkill to have 2 tags.

The complete list of possible values are :

  • operation tag: only 2 values — "execute" and "authenticate"

    • operation_name tag: "remove", "rename", "search", "searchPaginated", "lookupById", "modify", "create", "passwordModify", "unknown" (default)

    I agree to your point . Do u suggest that to merge them into a single "operation" tag where "authenticate" is one value alongside the specific LDAP operation types

Yes, that's my suggestion.

@ruchikajha95
Added the LDAP operation type and error name in the metric
622e93f 
Closes keycloak#50958

Signed-off-by: Ruchika <ruchika.jha1@ibm.com>
@ruchikajha95
Resolved the pr review comments
d8bf870 
Closes keycloak#50958

Signed-off-by: Ruchika <ruchika.jha1@ibm.com>
@ruchikajha95
Resolved the pr review comments
575650c 
Closes keycloak#50958

Signed-off-by: Ruchika <ruchika.jha1@ibm.com>
@ruchikajha95
ruchikajha95 force-pushed the feature-50958/operation-type-and-error-name-to-LDAP branch from b0f709f to 575650c Compare August 10, 2026 11:26
@ruchikajha95
changed the operation tags
1af4829 
Closes keycloak#50958

Signed-off-by: Ruchika <ruchika.jha1@ibm.com>
@keycloak-github-bot

keycloak-github-bot Bot commented Aug 10, 2026

Copy link
Copy Markdown

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.adapter.servlet.SAMLServletAdapterTest#salesPostEncSignedAssertionsOnlyTest

Keycloak CI - Adapter IT Strict Cookies

org.opentest4j.AssertionFailedError: 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

keycloak-github-bot[bot]
keycloak-github-bot Bot reviewed Aug 10, 2026
View reviewed changes

@keycloak-github-bot keycloak-github-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

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

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@pruivo pruivo Awaiting requested review from pruivo
@ahus1 ahus1 Awaiting requested review from ahus1
+1 more reviewer
@keycloak-github-bot keycloak-github-bot[bot] keycloak-github-bot[bot] left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

@ruchikajha95 ruchikajha95

Labels

flaky-test team/core-iam

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add operation type and error name to LDAP execute trace spans

4 participants

@ruchikajha95 @pruivo @stianst