-
Notifications
You must be signed in to change notification settings - Fork 18.9k
Switch all logging to use containerd log pkg #45799
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
ab113b7 to
6f5eaec
Compare
|
This is mostly a grep->sed to replace things but needed some help to clean things up after # 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
doneFound goimports was giving me the wrong import for logrus... which was really strange. See the script above for that correction as well (hint: |
|
Also really fun... |
83c691c to
5b96f4b
Compare
This unifies our logging and allows us to propagate logging and trace contexts together. Signed-off-by: Brian Goff <[email protected]>
5b96f4b to
74da6a6
Compare
|
All green. |
|
So really #45799 (comment) was not enough and go tooling is so bad for this. |
|
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 A couple of things that I'm wondering (also with in the back of my mind that
|
I'm not too worried about that here especially since it will be trivial to change the imports if needed after this is in.
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". |
|
Related (from long ago; probably should be closed, but still attacking a related issue): #36155 |
thaJeztah
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
For reference, we discussed changing Here's the incantation to make that happen, though: |
|
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. |
This unifies our logging and allows us to propagate logging and trace contexts together.