image: deprecate IDFromDigest(), and some minor fixes/cleanup#44426
image: deprecate IDFromDigest(), and some minor fixes/cleanup#44426thaJeztah merged 5 commits intomoby:masterfrom
Conversation
vvoland
left a comment
There was a problem hiding this comment.
+1 on using structured logs. I have some suggestions on DRYing the WithField calls.
Let me know what you think.
c225973 to
5a95032
Compare
|
@vvoland updated; ptal 👍 |
5a95032 to
d4fdf50
Compare
d4fdf50 to
1643db6
Compare
|
@corhere @neersighted ptal |
image/store.go
Outdated
| } | ||
| var l layer.Layer | ||
| if chainID := img.RootFS.ChainID(); chainID != "" { | ||
| logger = logger.WithField("chainID", chainID).WithField("os", img.OperatingSystem()) |
There was a problem hiding this comment.
Nit: chaining two (*logrus.Entry).WithField calls is surprisingly inefficient compared to .WithFields as it is implemented in terms of the latter and deep-copies the Entry on each call.
| logger = logger.WithField("chainID", chainID).WithField("os", img.OperatingSystem()) | |
| logger = logger.WithFields(logrus.Fields("chainID": chainID, "os": img.OperatingSystem())) |
Benchmarks
goos: darwin
goarch: amd64
pkg: github.com/corhere/logbench
cpu: Intel(R) Core(TM) i9-8950HK CPU @ 2.90GHz
BenchmarkWithField-12 10000 344272 ns/op 338640 B/op 15 allocs/op
BenchmarkWithFields-12 1000000 2372 ns/op 566 B/op 3 allocs/op
PASS
package logbench
import (
"io"
"strconv"
"testing"
"github.com/sirupsen/logrus"
)
func init() {
logrus.SetOutput(io.Discard)
}
func BenchmarkWithField(b *testing.B) {
entry := logrus.NewEntry(logrus.StandardLogger())
b.ResetTimer()
b.ReportAllocs()
for i := 0; i < b.N; i++ {
entry = entry.WithField(strconv.Itoa(i), i)
}
entry.Info("hi")
}
func BenchmarkWithFields(b *testing.B) {
entry := logrus.NewEntry(logrus.StandardLogger())
fields := logrus.Fields{}
b.ResetTimer()
b.ReportAllocs()
for i := 0; i < b.N; i++ {
fields[strconv.Itoa(i)] = i
}
entry.WithFields(fields).Info("hi")
}There was a problem hiding this comment.
That's actually a good point here; this code is ran on ImageStore.restore(), which iterates over all images (which could be "many"). The logger was added to make the code a bit more DRY (see discussion with @vvoland above #44426 (comment)), but looking now, I don't think that's a good idea for this specific case;
- the logger is only used in error conditions (the code is treats these as non-fatal, but logs them instead)
- but the
loggerwould be constructed unconditionally, which means a lot of allocations for something that (in most cases) wouldn't be used.
Let me change it back to instantiate the logger.WithField(s) only if it's gonna be used. It's less "pretty" but probably good for this case.
Having this function hides what it's doing, which is just to type-cast to an image.ID (which is a digest). Using a cast is more transparent, so deprecating this function in favor of a regular typecast. Signed-off-by: Sebastiaan van Stijn <[email protected]>
This code below is run when restoring all images (which can be "many"), constructing the "logrus.WithFields" is deliberately not "DRY", as the logger is only used for error-cases, and we don't want to do allocations if we don't need it. A "f" type-alias was added to make it ever so slightly more DRY, but that's just for convenience :) Signed-off-by: Sebastiaan van Stijn <[email protected]>
To reduce some type-juggling :) Signed-off-by: Sebastiaan van Stijn <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
1643db6 to
179ca12
Compare
There was a problem hiding this comment.
Looks like a slight typo in the commit message of 179ca12 -- "wrapp"
Otherwise, LGTM.
The image store's used are an interface, so there's no guarantee that implementations don't wrap the errors. Make sure to catch such cases by using errors.Is. Signed-off-by: Sebastiaan van Stijn <[email protected]>
179ca12 to
d131147
Compare
|
Fixed the typo; let me bring this one in 👍 |
Having this function hides what it's doing, which is just to type-cast to an image.ID (which is a digest). Using a cast is more transparent, so deprecating this function in favor of a regular typecast.
I'm looking at potentially making this type a straight alias for digest.Digest (and possibly sunsetting it)