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 session recording access format to audit event #47307

Conversation

gabrielcorado
Copy link
Contributor

This is part of adding usage metrics for session recording access. This PR adds the field format to the audit event. Possible values are json, yaml, text, and pty.

To receive the value, we have to update the calls of StreamSessionEvents to receive an additional argument (format). This function emits the audit event but can be called in different scenarios (depending on the requested format). In some places, this new attribute won't be used; however, it is still required, given that we use the same interface.

changelog: Include the format field on the session.recording.access audit event with the format in which the recording was accessed.

@gabrielcorado gabrielcorado requested a review from greedy52 October 8, 2024 02:15
@@ -2434,10 +2434,11 @@ func (c *Client) DeleteAllNodes(ctx context.Context, namespace string) error {
}

// StreamSessionEvents streams audit events from a given session recording.
func (c *Client) StreamSessionEvents(ctx context.Context, sessionID string, startIndex int64) (chan events.AuditEvent, chan error) {
func (c *Client) StreamSessionEvents(ctx context.Context, sessionID string, startIndex int64, format string) (chan events.AuditEvent, chan error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think about putting the format inside the context instead of an explicit parameter?

I don't love making format a required argument because the stream session events API doesn't know anything about formats - that all happens downstream. Adding a required argument for something that's only used for some event metadata feels wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree with this (especially after seeing so many places requiring this parameter, which would be empty by default). I'll update it and make another PR (there will be fewer places to change).

@gabrielcorado
Copy link
Contributor Author

Closing in favor of #47309

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