Update logging calls to take advantage of structured logging #9454
Conversation
ghost
assigned 
logger/style_guide.md
Outdated
| ### Context Key Names | ||
|
|
||
| The key for the dynamic content in the context should be formatted in | ||
| camelCase. For acronyms, each letter of the acronym should match the |
There was a problem hiding this comment.
Overall, really great documentation and guidelines. It may seem pedantic, however, I would like you to consider the use of snake_case, for a couple of reasons:
- they are less ambiguous in some cases. For example
isIllicitIgloovsis_illicit_igloo - based on an empirical study using engineers, the researchers found it required more effort to comprehend the camel-case identifiers. I imagine that will be quiet relevant in a sea of log lines.
There was a problem hiding this comment.
I personally like using dashes, but I think last time I brought this up, someone requested camelCase. We should just put it to a vote. I'm not a big fan of snake case mostly because I think it looks ugly in output, but whichever one a plurality wants, I'll take.
| @@ -681,7 +686,7 @@ func (h *Handler) serveWrite(w http.ResponseWriter, r *http.Request, user meta.U | |||
| atomic.AddInt64(&h.stats.WriteRequestBytesReceived, int64(buf.Len())) | |||
|
|
|||
| if h.Config.WriteTracing { | |||
| h.Logger.Info(fmt.Sprintf("Write body received by handler: %s", buf.Bytes())) | |||
| h.Logger.Info("Write body received by handler", zap.ByteString("body", buf.Bytes())) | |||
There was a problem hiding this comment.
Do you know if these values are truncated? Repeated requests with a huge payload it could cause storage issues.
There was a problem hiding this comment.
I do not. It is probably not. I think that's a separate issue though.
| @@ -851,7 +856,7 @@ func (h *Handler) servePromWrite(w http.ResponseWriter, r *http.Request, user me | |||
| atomic.AddInt64(&h.stats.WriteRequestBytesReceived, int64(buf.Len())) | |||
|
|
|||
| if h.Config.WriteTracing { | |||
| h.Logger.Info(fmt.Sprintf("Prom write body received by handler: %s", buf.Bytes())) | |||
| h.Logger.Info("Prom write body received by handler", zap.ByteString("body", buf.Bytes())) | |||
There was a problem hiding this comment.
Do you know if these values are truncated? Repeated requests with a huge payload it could cause storage issues.
| @@ -389,8 +389,7 @@ func (m *Monitor) createInternalStorage() { | |||
| } | |||
|
|
|||
| if _, err := m.MetaClient.CreateDatabaseWithRetentionPolicy(m.storeDatabase, &spec); err != nil { | |||
| m.Logger.Info(fmt.Sprintf("failed to create database '%s', failed to create storage: %s", | |||
| m.storeDatabase, err.Error())) | |||
| m.Logger.Info("Failed to create storage", zap.String("db", m.storeDatabase), zap.Error(err)) | |||
There was a problem hiding this comment.
you will be interested in my PR, as I have added some common Field helpers in your logging package to ensure we use consistent keys in the log context. For example:
// Database returns a field for tracking the name of a database.
func Database(name string) zapcore.Field
// Database returns a field for tracking the name of a database.
func RetentionPolicy(name string) zapcore.Field
// Shard returns a field for tracking the shard identifier.
func Shard(id uint64) zapcore.Field
jsternberg
force-pushed
the
js-structured-logging
branch
3 times, most recently
from
d12843d to
791f7a7
Compare
Includes a style guide that details the basics of how to log.
jsternberg
force-pushed
the
js-structured-logging
branch
from
791f7a7 to
2bbd967
Compare
Includes a style guide that details the basics of how to log.