Skip to content

Conversation

@cpuguy83
Copy link
Member

This unifies our logging and allows us to propagate logging and trace contexts together.

@cpuguy83 cpuguy83 force-pushed the containerd_logrus branch from ab113b7 to 6f5eaec Compare June 23, 2023 19:25
@cpuguy83 cpuguy83 marked this pull request as ready for review June 23, 2023 19:26
@cpuguy83
Copy link
Member Author

cpuguy83 commented Jun 23, 2023

This is mostly a grep->sed to replace things but needed some help to clean things up after

git grep 'logrus\.' | grep -v vendor | grep '.go' | grep -v '//' | grep -v GetLevel | grep -v SetLevel | grep -v AddHook | grep -v '[aA-zZ]+Level' | awk -F':' '{ print $1 }' | xargs sed -i 's,logrus\.,log.G(ctx).,'
# function signature messed up
syntaxErr1='unexpected ( in parameter list; possibly missing comma or )'
syntaxErr2='syntax error: unexpected ( after top level declaration'

fix() {
    infile="${1}"
    # fix weird issue where goimports gives us the wrong import...
    # Note: this may not be propperly grouped after this
    grep "command-line-arguments" "${infile}" | bash -xc 'while IFS= read -r line; do IFS=: read -r file num extra <<<$line; sed -i "${num}s,command-line-arguments/endor/,," $file; done'

    # Replace ctx with context.TODO() because "ctx" doesn't exist
    grep "undefined: ctx" "${infile}" | bash -xc 'while IFS= read -r line; do IFS=: read -r file num extra <<<$line; sed -i "${num}s,ctx,context.TODO()," $file; done'

    # Fix our original sed broke things
    grep "log\.G(ctx)\.ErrorKey" "${infile}" | bash -xc 'while IFS= read -r line; do IFS=: read -r file num extra <<<$line; sed -i "${num}s,log\.G(ctx)\.ErrorKey,logrus.ErrorKey," $file; done'
    grep "log\.G(ctx)\.Level" "${infile}" | bash -xc 'while IFS= read -r line; do IFS=: read -r file num extra <<<$line; sed -i "${num}s,log\.G(ctx)\.Level,logrus.Level," $file; done'
    grep "log\.G(context\.TODO())\.ErrorKey" "${infile}" | bash -xc 'while IFS= read -r line; do IFS=: read -r file num extra <<<$line; sed -i "${num}s,log\.G(context\.TODO())\.ErrorKey,logrus.ErrorKey," $file; done'
    grep 'log\.G(context.TODO())\..*Level' "${infile}" | bash -xc 'while IFS= read -r line; do IFS=: read -r file num extra <<<$line; sed -i "${num}s,log\.G(context\.TODO())\.\(.*Level\),logrus.\1," $file; done'
    grep 'log\.G(ctx)\..*Level' "${infile}" | bash -xc 'while IFS= read -r line; do IFS=: read -r file num extra <<<$line; sed -i "${num}s,log\.G(ctx)\.\(.*Level\),logrus.\1," $file; done'
    grep 'log.G(context.TODO()).SetOutput' "${infile}" | bash -xc 'while IFS= read -r line; do IFS=: read -r file num extra <<<$line; sed -i "${num}s,log\.G(ctx)\.\(.*SetOutput\),logrus.\1," $file; done'
    grep 'log.G(context.TODO()).AddHook' "${infile}" | bash -xc 'while IFS= read -r line; do IFS=: read -r file num extra <<<$line; sed -i "${num}s,log\.G(ctx)\.\(.*AddHook\),logrus.\1," $file; done'
    grep 'etwlog\.G(context.TODO())' "${infile}" | bash -xc 'while IFS= read -r line; do IFS=: read -r file num extra <<<$line; sed -i "${num}s,etwlog\.G(context\.TODO()),etwlog," $file; done'
    grep 'log.G undefined (type *eventlog.Log has no field or method G)' "${infile}" | bash -xc 'while IFS= read -r line; do IFS=: read -r file num extra <<<$line; sed -i "${num}s,log\.G(ctx),log," $file; done'
    grep "${syntaxErr1}" "${infile}" | bash -xc 'while IFS= read -r line; do IFS=: read -r file num extra <<<$line; sed -i "${num}s,log\.G(ctx),logrus," $file; done'
    grep "${syntaxErr2}" "${infile}" | bash -xc 'while IFS= read -r line; do IFS=: read -r file num extra <<<$line; sed -i "${num}s,log\.G(ctx),logrus," $file; done'

    # fix goimports
    rm go.mod # goimports doesn't like the empty go.mod hack
    grep "undefined: context" "${infile}" | bash -xc 'while IFS= read -r line; do IFS=: read -r file extra <<<$line; goimports -w ./${file}; done'
    grep "undefined: logrus" "${infile}" | bash -xc 'while IFS= read -r line; do IFS=: read -r file extra <<<$line; goimports -w ./${file}; done'
    grep "imported and not used" "${infile}" | bash -xc 'while IFS= read -r line; do IFS=: read -r file extra <<<$line; goimports -w ./${file}; done'
}

tmp="$(mktemp)"

cleanup() {
    cat "${tmp}"
    rm -f "${tmp}"
}

trap 'cleanup' EXIT

for goos in linux windows; do
    while ! (
        printf '%s\n\n%s' 'module github.com/docker/docker' 'go 1.19' >"go.mod"
        GOOS="${goos}" GO111MODULE=on go build -mod=vendor -modfile=vendor.mod -gcflags='-e' ./cmd/dockerd 2>"${tmp}"
    ); do
        fix "${tmp}"
    done
done

Found goimports was giving me the wrong import for logrus... which was really strange. See the script above for that correction as well (hint: command-line-arguments/endor)
There was probably a couple of other cases that I manually fixed when I first started going through it.

@cpuguy83
Copy link
Member Author

Also really fun... -gcflags='e' should give me all compile errors but it wasn't working and hence the loop.

@cpuguy83 cpuguy83 changed the title [WIP] Switch all logging to use containerd log pkg Switch all logging to use containerd log pkg Jun 23, 2023
@cpuguy83 cpuguy83 force-pushed the containerd_logrus branch 5 times, most recently from 83c691c to 5b96f4b Compare June 23, 2023 23:49
@cpuguy83 cpuguy83 requested a review from tianon as a code owner June 23, 2023 23:49
This unifies our logging and allows us to propagate logging and trace
contexts together.

Signed-off-by: Brian Goff <[email protected]>
@cpuguy83 cpuguy83 force-pushed the containerd_logrus branch from 5b96f4b to 74da6a6 Compare June 24, 2023 00:23
@cpuguy83
Copy link
Member Author

All green.

@thaJeztah thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Jun 26, 2023
@cpuguy83
Copy link
Member Author

So really #45799 (comment) was not enough and go tooling is so bad for this.
I ended up resorting to using golangci-lint to get all errors.

@thaJeztah
Copy link
Member

In general, I'm a big "plus 1" to doing this (and removing the logrus from everywhere). I'm wondering if we should consider making the log package a separate module in containerd if it's gonna be used more outside of containerd itself.

A couple of things that I'm wondering (also with in the back of my mind that slog is becoming a thing);

@cpuguy83
Copy link
Member Author

I'm wondering if we should consider making the log package a separate module

I'm not too worried about that here especially since it will be trivial to change the imports if needed after this is in.

First of all, I went looking for the context-logger part of Golang's slog,

I can't say exactly why it was removed with certainty, but I do know there are those in the Go community (with a lot of sway) who do not like this pattern (they are wrong, but 🤷 🤣 😇 )

For slog in general, that is a much larger change and isn't really buying us anything except maybe some extra CPU time due to allocations (nor does it get us log+tracing propagation). I think this will be more "oh well go has a half-way decent logging story now in the stdlib we should use that at some point".

@neersighted
Copy link
Member

Related (from long ago; probably should be closed, but still attacking a related issue): #36155

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cpuguy83
Copy link
Member Author

For reference, we discussed changing logrus.Fields to log.Fields (type-alias in containerd).
Our checkout of containerd is not new enough for this.

Here's the incantation to make that happen, though:

git grep 'logrus\.Field' | grep -v vendor | awk -F':' '{ print $1 }' | xargs sed -i 's,logrus\.Fields,log.Fields,g'

@thaJeztah
Copy link
Member

thaJeztah commented Jun 26, 2023

We discussed using the log.Fields alias, but that's not yet in the 1.6 branch, so we should look at getting that backported.

@neersighted neersighted merged commit 8805e38 into moby:master Jun 26, 2023
@thaJeztah thaJeztah added this to the 25.0.0 milestone Jun 26, 2023
@cpuguy83 cpuguy83 deleted the containerd_logrus branch June 26, 2023 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/refactor PR's that refactor, or clean-up code status/2-code-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants