conn: change design of tarantool.Logger #518
Conversation
We have used the slog library, and now the messages are more informative. Closes #504
babyTsakhes
force-pushed
the
babyTsakhes/gh-504-change-design-of-tarantool-Logger
branch
from
d32547b to
348e8dc
Compare
| if err = conn.dial(ctx); err == nil { | ||
| // Atomically increase the reconnect count | ||
| // (use atomic operations for thread safety) | ||
| reconnects := atomic.AddUint32(&conn.reconnectCount, 1) - 1 |
There was a problem hiding this comment.
Why are you using atomics here? Reconnects cannot be in parallel and dirty reads are not a problem here.
Also, are you sure we need to log reconnect count here? We're creating separate field inside conn, but I don't see use-case here.
| } | ||
|
|
||
| // Method to reset the reconnect count | ||
| func (conn *Connection) resetReconnectCount() { |
| } | ||
|
|
||
| // Method for getting the current number of reconnects | ||
| func (conn *Connection) GetReconnectCount() uint32 { |
| cancel() | ||
|
|
||
| if err != nil { | ||
| // The error will most likely be the one that Dialer |
There was a problem hiding this comment.
Are you sure this commentary should be removed?
| IsInitial bool | ||
| } | ||
|
|
||
| func (e ReconnectFailedEvent) EventName() string { return "reconnect_failed" } |
There was a problem hiding this comment.
I'm sure that EventName() should not be a short tag, since it's logging mechanism. If you want - you may add separate 'short' tag as logging field, but log message should be a properly human-readable message.
If we want to process 'events', there's already another mechanism in connector called 'Notify'.
I've looked into old code and find this kind of logged messages:
1. connection failed
2. last connection failed, giving up // ?? i'm not sure we need separate error, simple (5/5) should give us a same information
3. unable to parse watch event
4. unsupported box.session.push
5. unexpected event
| } | ||
| } | ||
|
|
||
| func (l *SlogLogger) WithContext(ctx context.Context) *SlogLogger { |
There was a problem hiding this comment.
| func (l *SlogLogger) WithContext(ctx context.Context) *SlogLogger { | |
| func (l SlogLogger) WithContext(ctx context.Context) *SlogLogger { |
| } | ||
| } | ||
|
|
||
| func (l *SlogLogger) Report(event LogEvent, conn *Connection) { |
There was a problem hiding this comment.
| func (l *SlogLogger) Report(event LogEvent, conn *Connection) { | |
| func (l SlogLogger) Report(event LogEvent, conn *Connection) { |
| func NewSlogLogger(logger *slog.Logger) *SlogLogger { | ||
| if logger == nil { | ||
| logger = slog.Default() | ||
| } | ||
| return &SlogLogger{ | ||
| logger: logger, | ||
| ctx: context.Background(), | ||
| } | ||
| } |
There was a problem hiding this comment.
It's dead-simple builder wrapper, no need to use ptr here, imo:
| func NewSlogLogger(logger *slog.Logger) *SlogLogger { | |
| if logger == nil { | |
| logger = slog.Default() | |
| } | |
| return &SlogLogger{ | |
| logger: logger, | |
| ctx: context.Background(), | |
| } | |
| } | |
| func NewSlogLogger(logger *slog.Logger) SlogLogger { | |
| if logger == nil { | |
| logger = slog.Default() | |
| } | |
| return SlogLogger{ | |
| logger: logger, | |
| ctx: context.Background(), | |
| } | |
| } |
Also add commentary pls
| func (e baseEvent) baseAttrs() []slog.Attr { | ||
| attrs := []slog.Attr{ | ||
| slog.String("component", "tarantool.connection"), | ||
| slog.Time("time", e.time), |
There was a problem hiding this comment.
Do not add time, we're using logger that user passes to us and if he want to see time - he will add time. Add only necessary fields.
If you mean time when we've created physical event - it should be called eventTime. But again - not sure this is really needed, diff between eventTime and "log time" should be very small.
| close(conn.control) | ||
| atomic.StoreUint32(&conn.state, connClosed) | ||
| conn.cond.Broadcast() | ||
| // Free the resources. |
There was a problem hiding this comment.
Why are you removing this code?
|
|
||
| select { | ||
| case <-ctx.Done(): | ||
| // Since the context is cancelled, we don't need to do anything. |
There was a problem hiding this comment.
Are you sure this commentary should be removed?
| } | ||
|
|
||
| func (l *SlogLogger) Report(event LogEvent, conn *Connection) { | ||
| attrs := event.LogAttrs() |
There was a problem hiding this comment.
Duplicate fields here? For example LogAttrs() returns max_reconnects and then we're appending max_reconnects from conn again.
conn: change design of tarantool.Logger
Closes #504 .