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

feat: Add ACL LOG #2536

Merged
merged 25 commits into from
May 8, 2023
Merged

feat: Add ACL LOG #2536

merged 25 commits into from
May 8, 2023

Conversation

SoulPancake
Copy link
Contributor

Resolves #2524

@SoulPancake SoulPancake marked this pull request as draft April 4, 2023 04:49
@SoulPancake SoulPancake marked this pull request as ready for review April 21, 2023 12:33
@SoulPancake
Copy link
Contributor Author

@monkey92t Can you please review this

command.go Outdated Show resolved Hide resolved
@monkey92t
Copy link
Collaborator

@SoulPancake We need to wait for the merge of #2483. In this PR, the ClientInfo field's type comes from #2483. According to our development guidelines, #2483 requires approval from other maintainers (developers cannot approve their own PR).

@monkey92t
Copy link
Collaborator

We can first think about the solutions for commands such as #2492, #2553, #2551.

@monkey92t
Copy link
Collaborator

@SoulPancake #2483 merged....please continue....

@SoulPancake
Copy link
Contributor Author

Can you take a look now @monkey92t ?

@monkey92t
Copy link
Collaborator

Can you take a look now @monkey92t ?

.........

I mentioned earlier, in #2483, we defined the ClientInfo struct. ACLLogEntry.ClientInfo should use ClientInfo instead of the string type. We already have the parseClientInfo method to parse it.

output:

127.0.0.1:6379> acl log 1
1)  1# "count" => (integer) 1
    2# "reason" => "auth"
    3# "context" => "toplevel"
    4# "object" => "AUTH"
    5# "username" => "someuser"
    6# "age-seconds" => (double) 192531.833
    7# "client-info" => "id=2351 addr=127.0.0.1:52326 laddr=127.0.0.1:6379 fd=8 name= age=1447772 idle=0 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 qbuf=48 qbuf-free=16842 argv-mem=25 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=18753 events=r cmd=auth user=default redir=-1 resp=2 lib-name=mylibname lib-ver=mylibver"
    8# "entry-id" => (integer) 0
    9# "timestamp-created" => (integer) 1682153764713
   10# "timestamp-last-updated" => (integer) 1682153764713

@monkey92t
Copy link
Collaborator

str: 7# "client-info" => "id=2351 addr=127.0.0.1:52326 laddr=127.0.0.1:6379 fd=8 name= age=1447772 idle=0 flags=N db=0 sub=0 psub=0 ssub=0 multi=-1 qbuf=48 qbuf-free=16842 argv-mem=25 multi-mem=0 rbs=1024 rbp=0 obl=0 oll=0 omem=0 tot-mem=18753 events=r cmd=auth user=default redir=-1 resp=2 lib-name=mylibname lib-ver=mylibver"

We should use the ClientInfo struct defined in #2482 for parsing, instead of using a string. This is why I asked you to wait for the #2482 merge.

// ClientInfo is redis-server ClientInfo, not go-redis *Client
type ClientInfo struct {
	ID                 int64         // redis version 2.8.12, a unique 64-bit client ID
	Addr               string        // address/port of the client
	LAddr              string        // address/port of local address client connected to (bind address)
	FD                 int64         // file descriptor corresponding to the socket
	Name               string        // the name set by the client with CLIENT SETNAME
	Age                time.Duration // total duration of the connection in seconds
	Idle               time.Duration // idle time of the connection in seconds
	Flags              ClientFlags   // client flags (see below)
	DB                 int           // current database ID
	Sub                int           // number of channel subscriptions
	PSub               int           // number of pattern matching subscriptions
	SSub               int           // redis version 7.0.3, number of shard channel subscriptions
	Multi              int           // number of commands in a MULTI/EXEC context
	QueryBuf           int           // qbuf, query buffer length (0 means no query pending)
	QueryBufFree       int           // qbuf-free, free space of the query buffer (0 means the buffer is full)
	ArgvMem            int           // incomplete arguments for the next command (already extracted from query buffer)
	MultiMem           int           // redis version 7.0, memory is used up by buffered multi commands
	BufferSize         int           // rbs, usable size of buffer
	BufferPeak         int           // rbp, peak used size of buffer in last 5 sec interval
	OutputBufferLength int           // obl, output buffer length
	OutputListLength   int           // oll, output list length (replies are queued in this list when the buffer is full)
	OutputMemory       int           // omem, output buffer memory usage
	TotalMemory        int           // tot-mem, total memory consumed by this client in its various buffers
	Events             string        // file descriptor events (see below)
	LastCmd            string        // cmd, last command played
	User               string        // the authenticated username of the client
	Redir              int64         // client id of current client tracking redirection
	Resp               int           // redis version 7.0, client RESP protocol version
	LibName            string        // redis version 7.2, client library name
	LibVer             string        // redis version 7.2, client library version
}

@SoulPancake
Copy link
Contributor Author

@monkey92t Gotchu.
Thanks! Working on migrating this

@monkey92t
Copy link
Collaborator

It is recommended to use pointers for large structures.

command.go Outdated Show resolved Hide resolved
commands.go Outdated Show resolved Hide resolved
command.go Outdated Show resolved Hide resolved
@SoulPancake
Copy link
Contributor Author

@vmihailenco @chayim @monkey92t
Please review once
and suggest if I should separate the ACL LOG command with no additional params

@chayim
Copy link
Contributor

chayim commented May 3, 2023

@elena-kolevska I think this is yours.

@chayim chayim added the feature label May 3, 2023
@elena-kolevska
Copy link
Contributor

elena-kolevska commented May 4, 2023

I think we should separate the two commands because even though they are the same command in Redis, logically, they do very different things and return very different responses.
I suggest that AclLog gets a single int argument, and we create a new command AclLogReset, for the log resetting.

@SoulPancake
Copy link
Contributor Author

I suggest that AclLog gets a single int argument
How will be able to cover the scenario where the single int arg is not provided or just "ACL LOG" is run with no additional params? @elena-kolevska

@elena-kolevska
Copy link
Contributor

Sorry @SoulPancake I saw monkey's comment and responded without seeing that further down you implemented the reset command. Let me look at it in more detail.

command.go Outdated Show resolved Hide resolved
@monkey92t
Copy link
Collaborator

monkey92t commented May 6, 2023

I think:

ACLLog(ctx context.Context, count int64) *ACLLogCmd
ACLLogReset(ctx context.Context) *StatusCmd

We can ignore the acl log command

@SoulPancake
Copy link
Contributor Author

I think:

ACLLog(ctx context.Context, count int64) *ACLLogCmd
ACLLogReset(ctx context.Context) *StatusCmd

We can ignore the acl log command

Sure, I can make that change
@elena-kolevska You good with this?

@monkey92t
Copy link
Collaborator

Here is my point of view:

  1. acl log is not a commonly used command.
  2. acl log is an abbreviation of acl log 10 and does not have any other meaning.
  3. ACLLog(ctx, count ...int64) can mislead people into thinking that multiple count parameters can be passed.

monkey92t
monkey92t previously approved these changes May 6, 2023
Copy link
Collaborator

@monkey92t monkey92t left a comment

Choose a reason for hiding this comment

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

I think this is good, and I look forward to hearing others' opinions.

@elena-kolevska
Copy link
Contributor

elena-kolevska commented May 6, 2023

The API looks good to me too. I would just add a doc comment saying that a reasonable value for count is 10, and that's the Redis default value.

Signed-off-by: monkey92t <golang@88.com>
Copy link
Collaborator

@monkey92t monkey92t left a comment

Choose a reason for hiding this comment

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

good~

@monkey92t monkey92t merged commit 31ba855 into redis:master May 8, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support ACL LOG: Added entry ID, timestamp created, and timestamp last updated.
5 participants