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 acctz counters for login/logout records #1236

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nmahabaleshwar
Copy link

@nmahabaleshwar nmahabaleshwar commented Dec 24, 2024

Add new counters for acctz records generated for login and logout records

Change Scope

  • Added source counters which will be incremented when login and logout records are generated.
  • A new type called session-service is added which defines new enums LOGIN and LOGOUT.
  • service-request has a new enum SESSION_SERVICE
  • This change is not backwards compatible.

For implementations which envelope LOGIN/LOGOUT record for every OPERATION record, we need a specific accounting source counters. Login/Logout events are generic and they cannot always be tied to an operation. Hence this PR introduces counters specific source counters for LOGIN and LOGOUT events.

This is also helpful when a LOGIN record is generated for a failed authentication event where we had ambiguity on which source counter to increment. This is now resolved with a specific counter for the LOGIN event.

For implementations which account using ONCE, we already have specific gRPC/Command counters for the intended operation.

Platform Implementations

None

@nmahabaleshwar nmahabaleshwar requested a review from a team as a code owner December 24, 2024 20:07
@dplore
Copy link
Member

dplore commented Dec 26, 2024

/gcbrun

@OpenConfigBot
Copy link

OpenConfigBot commented Dec 26, 2024

No major YANG version changes in commit ba3f4bf

@haussli
Copy link

haussli commented Dec 27, 2024

Did we discuss how/if to account for SESSION_STATUS_ONCE, et al?

I think that the commit message should be more verbose; it should describe the reason for this change, please.

@nmahabaleshwar
Copy link
Author

Did we discuss how/if to account for SESSION_STATUS_ONCE, et al?

I think that the commit message should be more verbose; it should describe the reason for this change, please.

Did you mean the counter update for SESSION_STATUS_ONCE ? SESSION_STATUS_ONCE already has information about which operation is being accounted, gRPC vs command services and we already have counters for them.

I will update the commit description with more details on why this change is required.

@dplore
Copy link
Member

dplore commented Jan 2, 2025

/gcbrun

@haussli
Copy link

haussli commented Jan 6, 2025

Did we discuss how/if to account for SESSION_STATUS_ONCE, et al?
I think that the commit message should be more verbose; it should describe the reason for this change, please.

Did you mean the counter update for SESSION_STATUS_ONCE ? SESSION_STATUS_ONCE already has information about which operation is being accounted, gRPC vs command services and we already have counters for them.

I will update the commit description with more details on why this change is required.

OK, understood; if login succeeds, the service type is known for the following record.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Ready to discuss
Development

Successfully merging this pull request may close these issues.

4 participants