-
Notifications
You must be signed in to change notification settings - Fork 642
CASSGO-9 Add more logging and a Structured Logger interface with log levels #1755
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
Conversation
|
Maybe it is better to move the extension modules to packages instead, I've raised this in #1770 (comment) |
|
Hello, @joao-r-reis! I like the proposal. Debugging gocql is a real headache sometimes. Getting rid of the The new API looks good to me. It lacks Trace() but I don't think we need this. Anyway, it can be handled on the driver side by just adding corresponding fields.
Yeah, this is a shame. It would be cool if it depended on the existing logging interfaces as slog. We should actually decide the go version gocql will be bumped to. |
This is an interesting suggestion but personally hammering out the details of that would probably be a bit complex so I'd rather leave that out of scope of this PR. Maybe it's a good enhancement idea for the future? |
I've done this justfyi |
I'm hesitant to change anything major without considering how it will work with
What is the reason for keeping the printf verbs in the messages? Do we need to maintain backwards compatibility with old log messages? |
In this PR I kept the old logger printf based interface as deprecated and my reasoning was that this is something that is probably widely used and breaking it could mean users feel discouraged to upgrade to 2.0. I'm open to the idea of removing the old logger though...
My |
|
I'm going to fix the conflicts today or tomorrow, this PR and the external type definitions PR are the two largest blocks that we have before we can move forward with the release |
I see what you mean but I'm concerned that if we switch logging and keep them in there then we're just continuing the dependence. I don't really see a great solution since we want to keep the existing logs unless we want to add 2 log calls to every spot where we log. Having the log message as |
|
I think I'll just replace the interface instead of adding a new one to use this opportunity that we're releasing 2.0. Users that rely on the current logger interface shouldn't find it too hard to adapt their implementation to the new interface |
cf19dad to
1a7382f
Compare
The logger interface uses printf semantics and lacks the notion of log levels. This commit changes (breaking change) the logger interface so printf semantics are removed in favor of structured logging and adds log levels. It provides 3 built in log implementations: - Default logger - uses log standard library - gocqlzap - uses zap library - gocqlzerolog - uses zerolog library Users can use these implementations as is or they can implement their own using these as examples. Patch by João Reis; reviewed by James Hartig, Bohdan Siryk for CASSGO-9
JIRA
Motivation
In my experience troubleshooting connection issues in gocql is quite complicated because there's not enough logging information available to understand what is going on under the hood (control connection reconnections, nodes being considered UP/DOWN, nodes being added or removed, connection pool errors, etc.). There is already some logging for this in place at the moment but it requires a build flag to be set which is far from ideal.
Adding logging for all of this by default would make the default gocql experience a bit frustrating for users that do not care about all of this extra information so log levels are also needed. With log levels, one can make the logging more verbose if they are trying to troubleshoot a particular issue. Other C* drivers already provide this kind of functionality out of the box.
Proposal
As mentioned in the prior section, with this PR I'm adding logging for a lot of the scenarios that were described above and I'm also adding log levels so that log verbosity can be tuned by users.
On top of the additional logging and log levels, I'm also adding better support for structured logging (new
StructuredLoggerinterface and field in the cluster config). Note that the current "printf" way needs to be supported at the very least until gocql v2 to maintain the same behavior that exists today in apps that are leveraging theStdLoggerinterface.I'm proposing to set the default log level as
WARNINGbecause it looks likeWARNINGis the most appropriate default level taking into account what logging there is today in gocql by default (prior to this proposal).It would be great if other community members could chime in on this proposal to improve any weak points that it might have and to make sure it addresses the needs of our users.
Usage/Examples
Example application side code with zerolog:
Alternatives
I considered
log/sloginstead of creating a new Structured Logger interface from scratch butlog/slogwas only introduced ingo 1.21so I discarded this option for now. Maybe it would be a good idea to move tolog/slogdown the line when a new major version of gocql is being worked on?Drawbacks
The biggest drawback in this solution is that in order for a user to take full advantage of the new StructuredLogger interface, they have to implement a new type while this wasn't needed in the case of
StdLoggerfor some logging libraries.I've added 2 new "extension" modules to provide an out of the box implementation of this new logger interface with
zerologandzapbut I'm not sure if this would be the best approach to provide this functionality.Log output examples
The following examples were collected following one simple procedure with CCM where:
Each log sample was collected on a unique run of this procedure so these log samples might not be fully comparable with each other.
Zerolog with gocql minimum log level set to WARN
Default logger with gocql minimum log level set to WARN
Zerolog with gocql minimum log level set to DEBUG
Default logger with gocql minimum log level set to DEBUG