Skip to content

Changing logging to use logrus.WithFields#387

Merged
jterry75 merged 1 commit intomasterfrom
log_kvp
Dec 1, 2018
Merged

Changing logging to use logrus.WithFields#387
jterry75 merged 1 commit intomasterfrom
log_kvp

Conversation

@jterry75
Copy link
Copy Markdown
Contributor

Signed-off-by: Justin Terry (VM) juterry@microsoft.com

@jterry75 jterry75 requested review from jstarks and lowenna November 21, 2018 22:08
@jterry75
Copy link
Copy Markdown
Contributor Author

Not ready for this to merge still have a lot of work to do in the other packages but before I go crazy I want some early feedback on the approach and if you think this will be helpful to do it in this way.

Comment thread internal/hcs/watcher.go
@jterry75
Copy link
Copy Markdown
Contributor Author

@kevpar - FYI

Comment thread internal/copywithtimeout/copywithtimeout.go Outdated
@jstarks
Copy link
Copy Markdown
Member

jstarks commented Nov 21, 2018

The example in https://godoc.org/github.com/sirupsen/logrus shows using WithFields for multi-field logging, not individual WithField calls. Which makes sense, because WithFields appears to be more efficient.

@jstarks
Copy link
Copy Markdown
Member

jstarks commented Nov 21, 2018

Also, WithError just adds a field called "error" (or alternatively, logrus.ErrorKey), so we don't need to use it separately if there are other fields.

Comment thread internal/hcs/process.go Outdated
Comment thread internal/hcs/process.go Outdated
Comment thread internal/hcs/process.go Outdated
Comment thread internal/hcs/watcher.go Outdated
@jterry75
Copy link
Copy Markdown
Contributor Author

jterry75 commented Nov 22, 2018

@jstarks - I don't mind if we write logrus.ErrorKey into fields ourselves if we are already creating the logrus.Fields map previous to calling the log message.

In practice I feel like I want to write code like this (obviously more polished):

func DoWork(ctx context.Context, ...) error {
    ctx := log.Start(ctx, "DoWork")
    defer log.End(ctx, "DoWork")

    // Do something
    if err := ChildOperation(ctx); err != nil {
        return err
    }

    // ctx is restored to the original DoWork context
    SameContext(ctx)
}

func SubOperation(ctx context.Context) error {
    // Start a subcontext that tracks its parent but does not affect it.
    ctx := log.Start(ctx, "SubOperation")
    defer log.End(ctx, "SubOperation")

    id = "test" // RealWork that creates id
    ctx.WithFields("id", id)

    log.Debug(ctx, "Success")
    return nil
}

func SameContext(ctx context.Context) {
    ctx.WithFields("flows-up", true)
    log.Debug(ctx, "SameContext - Success")
}

And then see output like this:

{ "msg": "DoWork: Start Operation" }
{ "msg": "SubOperation: Start Operation", "parent": "DoWork" }
{ "msg": "SubOperation: Success", "id": "test", "parent": "DoWork" }
{ "msg": "SubOperation: End Operation", "id": "test", "parent": "DoWork" }
{ "msg": "DoWork: SameContext - Success", "flows-up": true }
{ "msg": "DoWork: End Operation", "flows-up": true }

@jterry75 jterry75 force-pushed the log_kvp branch 3 times, most recently from 0c7b3c0 to c45fa5a Compare November 24, 2018 21:41
Comment thread cmd/runhcs/container.go Outdated
Comment thread internal/hcs/watcher.go
Comment thread internal/hcs/watcher.go Outdated
Comment thread internal/logfields/fields.go
@jterry75 jterry75 force-pushed the log_kvp branch 2 times, most recently from 5fb9ae2 to 11441d8 Compare November 28, 2018 22:52
@jterry75
Copy link
Copy Markdown
Contributor Author

@jstarks @jhowardmsft - PTAL. I removed any additional syscallWatcher that I added. I will wait until #388 to fix these up if we decide to take that. I also changed process and system to use a statically created logctx field rather than alloc the map for each syscall begin/end which simplifies the maps nicely. I think this is good to go for at least these functions. Still need to convert internal/uvm and internal/hcsoci to this style for the best coverage.

Signed-off-by: Justin Terry (VM) <juterry@microsoft.com>
@jterry75 jterry75 changed the title WIP: Changing logging to use logrus.WithFields Changing logging to use logrus.WithFields Dec 1, 2018
@jterry75
Copy link
Copy Markdown
Contributor Author

jterry75 commented Dec 1, 2018

Merging this for now. Will add two additional changes for internal/uvm and internal/hcsoci logging changes.

@jterry75 jterry75 merged commit 67ddf37 into master Dec 1, 2018
@jterry75 jterry75 deleted the log_kvp branch December 1, 2018 04:49
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 this pull request may close these issues.

2 participants