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

Add keycloak online/offline sessions records #74

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jermarchand
Copy link

Motivation

Add online/offline sessions by realm and clientId. Similar to #51

What

Add 2 Gauge :

  • keycloak_online_sessions
  • keycloak_offline_sessions

Why

Show the number of sessions

How

On each user/admin events, put in a Gauge the result of the getActiveClientSessionStats.

Verification Steps

Add the steps required to check this change. Following an example.

  1. Build the SPI from this branch and start Keycloak with it.
  2. Login with users
  3. Open the metrics endpoint in a browser.

Checklist:

  • Code has been tested locally by PR requester
  • Changes have been successfully verified by another team member

Progress

  • Finished task

Additional Notes

I'm a newby with Prometheus metrics, so fell free to comment and propose better implementation.

The getActiveClientSessionStats contains only ClientId with active sessions. When all sessions of this client are logout, I don't found a solution to set the metric to 0.0.

@@ -31,13 +41,32 @@ public void onEvent(Event event) {
default:
PrometheusExporter.instance().recordGenericEvent(event);
}

setSessions(session.realms().getRealm(event.getRealmId()));
Copy link
Contributor

Choose a reason for hiding this comment

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

does this have to be executed for every event? Maybe there are certain events that indicate a change in sessions like logins / logouts?

Copy link
Author

Choose a reason for hiding this comment

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

Our Keycloak cluster is not configured to replicate sessions on all nodes.

If the Login is on one node, and the logout on an other. Until a new login or logout on the fist node, the sessions metrics is not updated for this node.

It's the reason why there is no filter on event's type.
I don't found a better solution to update the metrics in our cluster.

Comment on lines 184 to 185
Map<String,Long> map1 = Map.of("account", new Long(10), "admin-cli", new Long(0));
Map<String,Long> map2 = Map.of("account", new Long(0), "admin-cli", new Long(10));

Choose a reason for hiding this comment

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

  • sourceCompatibility = 1.8(build.gradle:20)
  • Map.of() is supported since Java 9

Copy link
Author

Choose a reason for hiding this comment

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

I rewrite it with :

        Map<String,Long> map1 = new HashMap<String, Long>() {{
            put("account", new Long(10));
            put("admin-cli", new Long(0));
        }};

Is it better ?

@pb82
Copy link
Contributor

pb82 commented Nov 16, 2020

@jermarchand I tried it and it seems to work, only one question: when does a session count as offline? I logged in and then closed the tab but the counter is still counting it as online:

keycloak_online_sessions{realm="user_realm",client_id="account",} 1.0

@jermarchand
Copy link
Author

Closing a tab does not send a disconnection request.
When a session is terminated due to "SSO Session Idle", no event is generated :(

When a ClientId has no more sessions it does not appear in the response of "getActiveClientSessionStats" so it might be necessary to send a metric to 0 for all existing clients ....

On one of our Keycloak clusters where we tested this metrics, we had a crash for several reasons (mem + network + ..).
Today we do not know if one of these reasons is to have called too much the "getActiveClientSessionStats" which is not stable when there are more than 500,000 sessions.

I don't know if this PR should be merged until someone do more tests.

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.

None yet

3 participants