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 online and offline sessions metrics #150

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

Conversation

OleksandrMishchuk
Copy link

@OleksandrMishchuk OleksandrMishchuk commented Jan 10, 2023

Motivation

There's no metrics exported for online and offline sessions in KC currently and that is pretty needed by business

What

Two gauges were added for onlineSessionCount and offlineSessionCount. Updates are done when LOGIN or LOGOUT events are catch and only for client where event happened, which reduces load on the system

Why

There's already one PR, which does the same, but from my point of view capturing of sessions is done incorrectly there and will definitely add extra load on a server as metrics are pushed on each event.

How

When LOGIN, CLIENT_LOGIN or LOGOUT event is catch we retrieve info of event's client sessions and push out metrics.

Verification Steps

  1. Login with several customers into KC
  2. Observe new metrics on /realms/{realm}/metrics endpoint
  3. Logout with some customers from KC
  4. Observe updated metrics on /realms/{realm}/metrics endpoint

Checklist:

  • Code has been tested locally by PR requester
  • New test cases are added
  • Changes have been successfully verified by another team member

Progress

  • Finished task

Additional Notes

@alexted
Copy link

alexted commented Nov 7, 2024

@OleksandrMishchuk Is this PR still relevant?

@OleksandrMishchuk
Copy link
Author

@OleksandrMishchuk Is this PR still relevant?

TBH, we are not using these metrics anymore on a project as we moved fully to online sessions, but I believe it could be pretty handy for those who use mixed sessions. I can fix merge conflicts when have a spare minute

@alexted
Copy link

alexted commented Nov 9, 2024

@OleksandrMishchuk Check out the conversation at #74 if you haven't seen it yet. Have you taken into account the wishes and suggestions expressed there?

@OleksandrMishchuk
Copy link
Author

@OleksandrMishchuk Check out the conversation at #74 if you haven't seen it yet. Have you taken into account the wishes and suggestions expressed there?

Checked that now and I'm a bit confused with implementation - in that PR we do a lot of redundant work of processing sessions that are not related to current event. Basically, we go through all online and offline sessions and update all gauges, while we can only update one related to current event. Maybe I didn't fully get the logic, idk.

I have now updated PR, so you are free to decide on what to do with it 😄
There were a lot of changes inside codebase after I made the original PR, so more changes were needed to properly integrate and test it

@alexted
Copy link

alexted commented Nov 11, 2024

Wow, super! Thank you so much for your work!
This is a very important functionality that was sorely lacking.
God bless you!

@alexted
Copy link

alexted commented Dec 23, 2024

@pb82 could you pay attention to this?

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.

2 participants