36478 storage-drivers log format standardization#36492
Merged
vdemeester merged 1 commit intomoby:masterfrom Apr 20, 2018
Merged
36478 storage-drivers log format standardization#36492vdemeester merged 1 commit intomoby:masterfrom
vdemeester merged 1 commit intomoby:masterfrom
Conversation
alejgh
changed the title
Codecov Report
@@ Coverage Diff @@
## master #36492 +/- ##
=========================================
Coverage ? 34.94%
=========================================
Files ? 613
Lines ? 45415
Branches ? 0
=========================================
Hits ? 15870
Misses ? 27449
Partials ? 2096 |
alejgh
changed the title
Contributor
|
@thaJeztah are we concerned about operators relying on the heterogenous keys when searching in their logging solutions? |
alejgh
force-pushed
the
36478-log-standardization
branch
from
d073f51 to
73ad006
Compare
GordonTheTurtle
added
the
dco/no
|
Please sign your commits following these rules: $ git clone -b "36478-log-standardization" git@github.com:alejgh/moby.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354205320
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -fAmending updates the existing PR. You DO NOT need to open a new one. |
Now all of the storage drivers use the field "storage-driver" in their log messages, which is set to name of the respective driver. Storage drivers changed: - Aufs - Btrfs - Devicemapper - Overlay - Overlay 2 - Zfs Signed-off-by: Alejandro GonzÃlez Hevia <alejandrgh11@gmail.com>
alejgh
force-pushed
the
36478-log-standardization
branch
from
23f3ebb to
9392838
Compare
GordonTheTurtle
removed
the
dco/no
Contributor
Author
|
@thaJeztah could you take a quick look, please? :) |
Member
|
Oh, sorry, this dropped off my radar: was discussing this with @tiborvass and looks like he's ok with the change. |
Contributor
|
Maybe each graph driver can have a logger instance and use it? This would eliminate the repetition of Something like this (for aufs): diff --git a/daemon/graphdriver/aufs/aufs.go b/daemon/graphdriver/aufs/aufs.go
index a46cb31d3..3e94a2f3e 100644
--- a/daemon/graphdriver/aufs/aufs.go
+++ b/daemon/graphdriver/aufs/aufs.go
@@ -62,6 +62,7 @@ var (
enableDirpermLock sync.Once
enableDirperm bool
+ logger = logrus.WithField("storage-driver", "aufs")
)
func init() {
@@ -109,7 +110,7 @@ func Init(root string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap
switch fsMagic {
case graphdriver.FsMagicAufs, graphdriver.FsMagicBtrfs, graphdriver.FsMagicEcryptfs:
- logrus.Errorf("AUFS is not supported over %s", backingFs)
+ logger.Errorf("AUFS is not supported over %s", backingFs)
return nil, graphdriver.ErrIncompatibleFS
}
@@ -143,11 +144,6 @@ func Init(root string, options []string, uidMaps, gidMaps []idtools.IDMap) (grap
return nil, err
}
}
- logger := logrus.WithFields(logrus.Fields{
- "module": "graphdriver",
- "driver": "aufs",
- })
-
for _, path := range []string{"mnt", "diff"} {
p := filepath.Join(root, path)
entries, err := ioutil.ReadDir(p)
@@ -309,11 +305,7 @@ func (a *Driver) Remove(id string) error {
mountpoint = a.getMountpoint(id)
}
- logger := logrus.WithFields(logrus.Fields{
- "module": "graphdriver",
- "driver": "aufs",
- "layer": id,
- })
+ logger := logger.WithField("layer", id)
var retries int
for {
diff --git a/daemon/graphdriver/aufs/mount.go b/daemon/graphdriver/aufs/mount.go
index 478198848..fbff1f147 100644
--- a/daemon/graphdriver/aufs/mount.go
+++ b/daemon/graphdriver/aufs/mount.go
@@ -5,14 +5,13 @@ package aufs // import "github.com/docker/docker/daemon/graphdriver/aufs"
import (
"os/exec"
- "github.com/sirupsen/logrus"
"golang.org/x/sys/unix"
)
// Unmount the target specified.
func Unmount(target string) error {
if err := exec.Command("auplink", target, "flush").Run(); err != nil {
- logrus.Warnf("Couldn't run auplink before unmount %s: %s", target, err)
+ logger.Warnf("Couldn't run auplink before unmount %s: %s", target, err)
}
return unix.Unmount(target, 0)
} |
Contributor
|
Optionally, rename |
Member
|
That's pretty much what I was thinking as well, but didn't want to make a huge diff to get this in.... but I guess then we'll just have two big diffs because all this will be re-written anyway. |
Contributor
Author
|
Yes, I thought about adding the logger instance to each graph driver, but I didn't want to mess it up somehow so I followed the previous style. I agree that now it makes a lot more sense to have the logger instance to avoid code duplication or in case this format has to be changed someday. |
Contributor
|
Experimental hits an unexpected error: Length is only 41 and the array ends with an empty string. |
Member
|
PowerPC (https://jenkins.dockerproject.org/job/Docker-PRs-powerpc/9411/console) is failing on a test that's flaky (mentioned in #33041) Janky failure (https://jenkins.dockerproject.org/job/Docker-PRs/48959/console) also is a flaky test (tracked through #36501) s390 actually passed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #36478
- What I did
Unify the properties of the log messages from the different storage drivers.
- How I did it
There were 4 different formats proposed/used for logging messages in the drivers:
logrus.WithField("storage-driver", "devicemapper").logrus.WithField("driver", "zfs").logrus.WithField("module", "graphdriver/devicemapper")was also proposedI used the second format (
logrus.WithField("storage-driver", "xxxxx")across all the storage drivers as it didn't seem to be a preference of one format over another, but I can change the format if needed.Most of the log messages in the storage-drivers didn't have any property assigned, so I added it to all of them.
- How to verify it
Every test regarding the module is passing, although there didn't seem to be any test regarding the output of the log messages so I didn't make any change to the tests. I don't know if I missed them.
- Description for the changelog
Standardized the properties of storage-driver log messages.
- A picture of a cute animal (not mandatory but encouraged)
I apologise if there is something I did wrong, this is my first time contributing. Any help would be appreciated. Thank you and sorry for the inconveniences 😔