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

[NEW] Splitting logfile for auditing #260

Open
LiiNen opened this issue Apr 8, 2024 · 4 comments · May be fixed by #261
Open

[NEW] Splitting logfile for auditing #260

LiiNen opened this issue Apr 8, 2024 · 4 comments · May be fixed by #261

Comments

@LiiNen
Copy link
Contributor

LiiNen commented Apr 8, 2024

The problem/use-case that the feature addresses

Valkey, for now, does not support logging files' variety.
In other databases, there are some use cases that splitting logs to each other files - like err.log, warning.log, and etc - , so that it helps audit DB instances.

Me either as a DBA in a company that provides Redis in internal cloud service, this log tracking is very important.
We have parameter 'LOGLEVEL', and this allows us to write more logs, but only in same "logfile" (ex. ./valkey.log).

I think this might hurt for checking logs, because the gap between logging levels is quite big.
So, because there already exists codes for log level, I have thought about splitting logfile into two: valkey.log and audit.log

Description of the feature

Simply, adding new parameter in config file, so that we can use another log file for lower logging level.
But default is LL_NOTICE, and LL_DEBUG is for debug literally. so finally, this is only for adding logfile for LL_VERBOSE.

/* Log levels */
#define LL_DEBUG 0
#define LL_VERBOSE 1
#define LL_NOTICE 2
#define LL_WARNING 3
#define LL_NOTHING 4
#define LL_RAW (1<<10) /* Modifier to log without timestamp */

I didn't intend to write code directly without core maintainers confirm.
But I needed to edit code for leaving splitted log files in my own work, so I also suggested this with some code. #261
(For make sure, this is not a company side work, just did it myself.)
Sorry for suggesting it directly with code, but think that this might help understanding what I said above.

Additional information

Note: I'd like to add log levels, but this is a quite big change, so I didn't do it for now.

/* TOBE (just my expectation) Log levels */
#define LL_DEBUG 0
#define LL_VERBOSE 1
#define LL_AUDIT 2
#define LL_NOTICE 3
#define LL_WARNING 4
#define LL_NOTHING 5
#define LL_RAW (1<<10) /* Modifier to log without timestamp */

thanks,

@LiiNen LiiNen linked a pull request Apr 8, 2024 that will close this issue
@enjoy-binbin
Copy link
Member

I think this might hurt for checking logs, because the gap between logging levels is quite big.

The server side usually does not have many logs, if there are, they are logs that require special attention. Did you have any trouble during the checking in the past?

@LiiNen
Copy link
Contributor Author

LiiNen commented Apr 9, 2024

Um, sorry for confusing. I meant it as below. I'll summarize it again.

whatever loglevel is set, ALL of the logs are written in valkey.log.
though we can filter it by some specific words, but I think if we can split the logs into with its purpose, then it will be fancy.

--

Also, just for suggestion, if the verbosity has more level, then it will be also useful
This suggestion is just my selfish thoughts, so you can ignore it if this concept is not suitable for valkey. lol
just wanted to get only client connection logs, without other verbose logs
Of course we can log and filter it with some specific words, but it may leads to large files with operating for a long time.

The server side usually does not have many logs

Think that VERBOSE is too noisy, cause it is executed every 5 sec. (even if it is literally 'verbose'.)
But VERBOSE is just next to NOTICE - the default verbosity.

/* Show some info about non-empty databases */
if (server.verbosity <= LL_VERBOSE) {
    run_with_period(5000) {
        for (j = 0; j < server.dbnum; j++) {
            long long size, used, vkeys;

            size = kvstoreBuckets(server.db[j].keys);
            used = kvstoreSize(server.db[j].keys);
            vkeys = kvstoreSize(server.db[j].expires);
            if (used || vkeys) {
                serverLog(LL_VERBOSE,"DB %d: %lld keys (%lld volatile) in %lld slots HT.",j,used,vkeys,size);
            }
        }
    }
}

@PingXie
Copy link
Member

PingXie commented Apr 13, 2024

@LiiNen, I see you touched upon a few points in the logging area (and you remind me of Windows's log manager)

  1. Logging levels

I can see AUDIT logging being useful in some cases. For instance, when a new user is added/deleted, etc. We can probably consider introducing this level. Though it branches the existing log levels so that they won't be monodically increasing anymore.

  1. Separate log files

I am not sure what value separate log files would bring. As @enjoy-binbin mentioned above, there isn't a lot of things to log today - the only exception is the cluster topology changes, which I do think we need to improve on next to help with operations. But even there I wouldn't expect us to add 100s or more log lines. A single log is still quite manageable IMO.

  1. Structured log

I do think the current Valkey log is way too free-formed, which makes parsing unnecessarily hard. However, this is unlike a priority right now, for the same reason that there just isn't a lot of things to log today. But, IF we ever got a chance to redo the logging:

  • First of all, I don't think we should remove the old log lines. They should be retained for "backward compat".

  • Then, we could consider a schema like below:

    a. identity of "engine" vs "modules"
    b. for "engine", the component file name, such as server.c/rdb.c/etc
    c. for "modules", the actual module name
    d. the entity identity, such as node/shard/ip/db num/slot/etc.

    the schema should also be extensible since the requirements can change and flexible schema would also allow us to evolve the logging gradually with smaller changes.

  • Lastly, we build on top of the schema our new richer logging utility functions.

@LiiNen
Copy link
Contributor Author

LiiNen commented Apr 14, 2024

@PingXie Thanks for your comments.

And thanks for comment about 1. Logging levels and 3. Structured log. I hope sometimes valkey branches the existing log levels with more fancy logs. This maybe quite a big change (even if it does not hurts any core operations), so I will wait this to be considered positive by your side (and also maintainers' side)

First of all, I don't think we should remove the old log lines. They should be retained for "backward compat".
Maybe this might be useless, because, as i know, there isn't any libraries or else that parse logfile. I think it is just for us-users that maintain server-so 'old log lines' could be removed after debating of it.

And lastly, for 2. Separate log files, agree to your disagree.
It really does not have giant logs, and I agree that single log has its advantages. But the point I wanted to tell is, if users can choose whether using a single logfile or multiple logfiles, then it could be nice in some part.

Anyway, thanks for your comment again and this issue could be closed if there is no plans to fix it in the near future.
I would be glad if this issue is mentioned when there are some move to re-design logging. (or else left this issue as open)

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 a pull request may close this issue.

3 participants