8000 ErrorId for error screens and logging by dafenix · Pull Request #44072 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

ErrorId for error screens and logging - #44072

Closed
dafenix wants to merge 1 commit into
keycloak:mainfrom
dafenix:feature/ErrorHandlerSpi
Closed

ErrorId for error screens and logging#44072
dafenix wants to merge 1 commit into
keycloak:mainfrom
dafenix:feature/ErrorHandlerSpi

Conversation

@dafenix
@dafenix dafenix commented Nov 10, 2025 •
edited by ahus1
Loading
Copy link
Copy Markdown

Related to #42004

Closes #44090

In case of an error, we want to display an ErrorId to the user, which they can provide for further support. To achieve this, it is necessary to intervene in the behavior of the ErrorHandler in order to set the ErrorId there and include it in the Freemarker template. I have already implemented a suggestion for this, which, in addition to the ErrorId, also sets a log marker that enriches the error entry in the log with the ErrorId.

Example:

CustomErrorResponseHandlerProvider.java

package org.keycloak.services.error;

import jakarta.ws.rs.core.Response;
import org.jboss.logging.Logger;
import org.jboss.logging.MDC;
import org.keycloak.models.KeycloakSession;

import java.util.Map;
import java.util.UUID;

import static org.keycloak.services.error.KeycloakErrorHandler.ERROR_RESPONSE_TEXT;
import static org.keycloak.services.error.KeycloakErrorHandler.UNCAUGHT_SERVER_ERROR_TEXT;

public class CustomErrorResponseHandlerProvider implements ErrorResponseHandlerProvider {
    private static final Logger logger = Logger.getLogger(CustomErrorResponseHandlerProvider.class);
    public static final String ERROR_ATTRIBUTE_ID = "errorId";
    public static final String ERROR_ID = "error.id";

    private String errorId;

    public CustomErrorResponseHandlerProvider(KeycloakSession session) {

    }

    @Override
    public void prepareErrorContext(boolean isServerError, Response.Status httpStatus, Throwable errorCause) {
        errorId = UUID.randomUUID().toString();

        if (MDC.get(ERROR_ID) == null) {
            MDC.put(ERROR_ID, errorId);
        }

        if (isServerError) {
            logger.error(UNCAUGHT_SERVER_ERROR_TEXT, errorCause);
        } else {
            logger.warnv(errorCause, ERROR_RESPONSE_TEXT, httpStatus);
        }
    }

    @Override
    public void enrichErrorPageModel(Map<String, Object> templateModel) {
        if (!templateModel.containsKey(ERROR_ATTRIBUTE_ID)) {
            templateModel.put(ERROR_ATTRIBUTE_ID, errorId);
        }
    }

    @Override
    public void close() {

    }
}

META-INF.services/org.keycloak.services.error.ErrorResponseHandlerProviderFactory

org.keycloak.services.error.CustomErrorResponseHandlerProviderFactory
image

@dafenix
dafenix requested a review from a team as a code owner November 10, 2025 11:27
@dafenix dafenix changed the title #42004 Added possibility to add a unique id to trace errors and enric… #42004 ErrorId for error screens and logging Nov 10, 2025
@dafenix
dafenix force-pushed the feature/ErrorHandlerSpi branch from d9e34e5 to 0d0ceb0 Compare November 10, 2025 12:06
@ahus1 ahus1 changed the title #42004 ErrorId for error screens and logging ErrorId for error screens and logging Nov 10, 2025
@ahus1
ahus1 commented Nov 10, 2025
Copy link
Copy Markdown
Member

Thank you for bringing this up. One formal comment to this: Please create a GitHub issue of type enhancement first that references the discussion. Then update this PR to reference the issue.

The enhancement you're adding here should be IMHO added to the default KeycloakErrorHandler and should depend on tracing being enabled.

Instead of creating a new UUID, it should instead use the current traceId. The advantage of this is that all logs that are connect to the error will already have that traceId in the logs.

Let me know your thoughts.

cc: @jbman, @tbrandstetter

@dafenix dafenix mentioned this pull request Nov 10, 2025
Closed
@dafenix
dafenix commented Nov 10, 2025 •
edited
Loading
Copy link
Copy Markdown
Author

Thank you for bringing this up. One formal comment to this: Please create a GitHub issue of type enhancement first that references the discussion. Then update this PR to reference the issue.

Done.

The enhancement you're adding here should be IMHO added to the default KeycloakErrorHandler...

I agree.

Instead of creating a new UUID, it should instead use the current traceId. The advantage of this is that all logs that are connect to the error will already have that traceId in the logs.
Let me know your thoughts.

Im not sure if this feature should depend on tracing. I mean isn't it useful for people which haven't a tracing system connected to Keycloak isn't it?

Furthermore, I think that from the end user's perspective, a shorter ID would be better in terms of readability and handling. Such IDs are often dictated over the phone.

@dafenix
dafenix force-pushed the feature/ErrorHandlerSpi branch from 0d0ceb0 to 39b36bf Compare November 25, 2025 14:02
@dafenix
dafenix requested a review from a team as a code owner November 25, 2025 14:02
@dafenix
dafenix commented Nov 25, 2025
Copy link
Copy Markdown
Author

I updated the code:

  • Removed the custom SPI and directly changed the KeycloakErrorHandler class
  • Instead of the UUID used a short error number which should be better in terms of readability and handling
  • Beside KeycloakErrorHandler also added new error id to FreeMarkerLoginFormsProvider. (Looks like these are the two locations where error handling is done)

Testing:

To invoke the KeycloakErrorHandler Error Screen you can simply use the wrong HTTP method for the token endpoint.
GET http://127.0.0.1:8080/realms/master/protocol/openid-connect/token
image

To test the FreeMarkerLoginFormsProvider you can call the auth endpoint with an unknown client-id:
http://127.0.0.1:8080/realms/master/protocol/openid-connect/auth?client_id=account-console22&redirect_uri=http://127.0.0.1:8080/realms/master/account&response_type=code

image

@dafenix
dafenix force-pushed the feature/ErrorHandlerSpi branch from 39b36bf to ea7cdac Compare November 25, 2025 15:10
@ahus1
ahus1 commented Nov 25, 2025
Copy link
Copy Markdown
Member

Thank you for the udpated PR.

I'm not sure if this feature should depend on tracing. I mean isn't it useful for people which haven't a tracing system connected to Keycloak isn't it?

You can run Keycloak with --tracing-sampler-ratio=0 and then the trace information will only show in the MDC, and you don't need a back end.

Looking at the code, the error ID is only generated when an error happens. I could see a benefit with a request ID, where the ID is generated at the very beginning of the request and added to the MDC all the time. This would allow you to collect all information connected to the request ID from the logs, even before the error happened. This would have been one of the advantages if the trace ID would be used, although I admit it is longer than the one you suggested.

Such a request ID could then also be added to all user and admin events created by Keycloak.

Some services also provide a request ID - see https://devcenter.heroku.com/articles/http-request-id for an article. So there might then be a configuration option to use the request ID, or to generate one ourselves.

WDYT?

@dafenix
dafenix commented Nov 26, 2025 •
edited
Loading
Copy link
Copy Markdown
Author

I'm not sure if this feature should depend on tracing. I mean isn't it useful for people which haven't a tracing system connected to Keycloak isn't it?

You can run Keycloak with --tracing-sampler-ratio=0 and then the trace information will only show in the MDC, and you don't need a back end.

Ok thats nice.

Looking at the code, the error ID is only generated when an error happens. I could see a benefit with a request ID, where the ID is generated at the very beginning of the request and added to the MDC all the time. This would allow you to collect all information connected to the request ID from the logs, even before the error happened. This would have been one of the advantages if the trace ID would be used, although I admit it is longer than the one you suggested.

Such a request ID could then also be added to all user and admin events created by Keycloak.

Some services also provide a request ID - see https://devcenter.heroku.com/articles/http-request-id for an article. So there might then be a configuration option to use the request ID, or to generate one ourselves.

I think a general request ID is a great idea. If i understand you correctly this doesn't exist at the moment. Do you know what would be the right place to add it to the context? I mean it should be at the very beginning of the request processing.

Updated the PR which now contains:

  • A kc.requestId for any request. Uses existing X-Request-ID header if present (from external systems)
  • X-Request Id is now part of every response in Keycloak
  • Replaced errorId with X-Request-ID for now.

Let me know what you think.

@dafenix
dafenix force-pushed the feature/ErrorHandlerSpi branch from ea7cdac to a2e23dd Compare November 26, 2025 15:41
@dafenix
dafenix requested a review from a team as a code owner November 26, 2025 15:41
@jbman
jbman commented Nov 26, 2025
Copy link
Copy Markdown
Contributor

@ahus1, I would keep the initial approach by @dafenix, to have a generated Error ID displayed to the user. It is important that the error is also logged and contains this ID. Any trace ID should be found in the same log line beside the Error ID.
The Error ID can be created just in time for the log line and for the screen to correlate both of them.

A trace ID provided within HTTP header X-Request-ID may be from a third party. You don't want to display that to a user.

Tracing should remain an independent aspect, so that Open Telemetry tracing could be supported and or X-Request-ID or any other trace propagation. The Error ID feature does not need to be aware of this.

@dafenix
42004 Added a short error id to trace errors and enrich log messages …
6483251 
…with it. Beside that this error can be presented to the user on the error page.

Signed-off-by: Daniel Weber <mail.weberdaniel@gmail.com>
@dafenix
dafenix force-pushed the feature/ErrorHandlerSpi branch from a2e23dd to 6483251 Compare February 4, 2026 08:16
@dafenix
dafenix commented Feb 4, 2026
Copy link
Copy Markdown
Author

Hey,

any updates on this?

@ahus1
ahus1 commented Feb 4, 2026
Copy link
Copy Markdown
Member

@dafenix - there have been different approaches discussed here with their advantages and disadvantages, and it requires some more discussion. I've added it to our roadmap planning. Eventually we will plan the parent issue for a milestone. Until then, this is on hold while we focus on enhancements on our roadmap.

@ahus1
ahus1 commented Mar 26, 2026
Copy link
Copy Markdown
Member

I consider the other PR which uses the trace ID good enough. Closing this one due to that.

@ahus1 ahus1 closed this Mar 26, 2026
@dafenix
dafenix commented Mar 27, 2026
Copy link
Copy Markdown
Author

I consider the other PR which uses the trace ID good enough. Closing this one due to that.

What do you think about a more readable error id like described in my opening post here? Is it something we should aiming for?

@ahus1
ahus1 commented Mar 27, 2026
Copy link
Copy Markdown
Member

What do you think about a more readable error id like described in my opening post here? Is it something we should aiming for?

Given the other items that are on our road map I would like to prioritize other things instead. So this is a no, I'm sorry.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

team/cloud-native

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ErrorId for error screens and logging

3 participants

@dafenix @ahus1 @jbman
0