volume/service: change some logs to use structured logs by thaJeztah · Pull Request #48675 · moby/moby

thaJeztah · 2024-10-16T10:50:35Z

- A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: Sebastiaan van Stijn <[email protected]>

AkihiroSuda · 2024-10-17T10:46:16Z

 		driverName := v.DriverName()
 		if _, err := s.drivers.ReleaseDriver(driverName); err != nil {
-			log.G(ctx).WithError(err).WithField("driver", driverName).Error("Error releasing reference to volume driver")
+			log.G(ctx).WithFields(log.Fields{"error": err, "driver": driverName, "volume-name": name}).Error("Error releasing reference to volume driver")


What was the problem with WithError?

Mostly because WithError, WithField, WithFields do a lot under the hood; they have to merge new values with old values, and performing reflection; doing a single WithFields can avoid doing that multiple times;

moby/vendor/github.com/sirupsen/logrus/entry.go

Lines 105 to 151 in 921ac59

// Add an error as single field (using the key defined in ErrorKey) to the Entry.

func (entry *Entry) WithError(err error) *Entry {

return entry.WithField(ErrorKey, err)

}

// Add a context to the Entry.

func (entry *Entry) WithContext(ctx context.Context) *Entry {

dataCopy := make(Fields, len(entry.Data))

for k, v := range entry.Data {

dataCopy[k] = v

}

return &Entry{Logger: entry.Logger, Data: dataCopy, Time: entry.Time, err: entry.err, Context: ctx}

}

// Add a single field to the Entry.

func (entry *Entry) WithField(key string, value interface{}) *Entry {

return entry.WithFields(Fields{key: value})

}

// Add a map of fields to the Entry.

func (entry *Entry) WithFields(fields Fields) *Entry {

data := make(Fields, len(entry.Data)+len(fields))

for k, v := range entry.Data {

data[k] = v

}

fieldErr := entry.err

for k, v := range fields {

isErrField := false

if t := reflect.TypeOf(v); t != nil {

switch {

case t.Kind() == reflect.Func, t.Kind() == reflect.Ptr && t.Elem().Kind() == reflect.Func:

isErrField = true

}

}

if isErrField {

tmp := fmt.Sprintf("can not add field %q", k)

if fieldErr != "" {

fieldErr = entry.err + ", " + tmp

} else {

fieldErr = tmp

}

} else {

data[k] = v

}

}

return &Entry{Logger: entry.Logger, Data: data, Time: entry.Time, err: fieldErr, Context: entry.Context}

}

@AkihiroSuda ☝️ does that answer your question? Do the changes LGTY?

AkihiroSuda · 2024-10-18T21:33:18Z

 		driverName := v.DriverName()
 		if _, err := s.drivers.ReleaseDriver(driverName); err != nil {
-			log.G(ctx).WithError(err).WithField("driver", driverName).Error("Error releasing reference to volume driver")
+			log.G(ctx).WithFields(log.Fields{"error": err, "driver": driverName, "volume-name": name}).Error("Error releasing reference to volume driver")


nit:

Suggested change

log.G(ctx).WithFields(log.Fields{"error": err, "driver": driverName, "volume-name": name}).Error("Error releasing reference to volume driver")

log.G(ctx).WithFields(log.Fields{logrus.ErrorKey: err, "driver": driverName, "volume-name": name}).Error("Error releasing reference to volume driver")

Ah, yeah, that's on purpose; trying to avoid direct imports of logrus (we changed all imports to use containerd's logs package, which provides aliases for the essential parts)

volume/service: change some logs to use structured logs

4e840b9

Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah added status/2-code-review area/volumes Volumes kind/refactor PR's that refactor, or clean-up code labels Oct 16, 2024

thaJeztah added this to the 28.0.0 milestone Oct 16, 2024

thaJeztah self-assigned this Oct 16, 2024

vvoland approved these changes Oct 16, 2024

View reviewed changes

AkihiroSuda reviewed Oct 17, 2024

View reviewed changes

AkihiroSuda reviewed Oct 18, 2024

View reviewed changes

AkihiroSuda approved these changes Oct 18, 2024

View reviewed changes

thaJeztah merged commit b0632b2 into moby:master Oct 18, 2024

thaJeztah deleted the volumes_more_structured_logs branch October 18, 2024 21:36

austinvazquez mentioned this pull request Oct 28, 2024

[25.0 backport] volume/mounts: fix anonymous volume not being labeled #48787

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

volume/service: change some logs to use structured logs#48675

volume/service: change some logs to use structured logs#48675
thaJeztah merged 1 commit intomoby:masterfrom
thaJeztah:volumes_more_structured_logs

thaJeztah commented Oct 16, 2024

Uh oh!

AkihiroSuda Oct 17, 2024

Uh oh!

thaJeztah Oct 17, 2024

Uh oh!

thaJeztah Oct 18, 2024

Uh oh!

AkihiroSuda Oct 18, 2024

Uh oh!

thaJeztah Oct 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	// Add an error as single field (using the key defined in ErrorKey) to the Entry.
	func (entry Entry) WithError(err error) Entry {
	return entry.WithField(ErrorKey, err)
	}

	// Add a context to the Entry.
	func (entry Entry) WithContext(ctx context.Context) Entry {
	dataCopy := make(Fields, len(entry.Data))
	for k, v := range entry.Data {
	dataCopy[k] = v
	}
	return &Entry{Logger: entry.Logger, Data: dataCopy, Time: entry.Time, err: entry.err, Context: ctx}
	}

	// Add a single field to the Entry.
	func (entry Entry) WithField(key string, value interface{}) Entry {
	return entry.WithFields(Fields{key: value})
	}

	// Add a map of fields to the Entry.
	func (entry Entry) WithFields(fields Fields) Entry {
	data := make(Fields, len(entry.Data)+len(fields))
	for k, v := range entry.Data {
	data[k] = v
	}
	fieldErr := entry.err
	for k, v := range fields {
	isErrField := false
	if t := reflect.TypeOf(v); t != nil {
	switch {
	case t.Kind() == reflect.Func, t.Kind() == reflect.Ptr && t.Elem().Kind() == reflect.Func:
	isErrField = true
	}
	}
	if isErrField {
	tmp := fmt.Sprintf("can not add field %q", k)
	if fieldErr != "" {
	fieldErr = entry.err + ", " + tmp
	} else {
	fieldErr = tmp
	}
	} else {
	data[k] = v
	}
	}
	return &Entry{Logger: entry.Logger, Data: data, Time: entry.Time, err: fieldErr, Context: entry.Context}
	}

	log.G(ctx).WithFields(log.Fields{"error": err, "driver": driverName, "volume-name": name}).Error("Error releasing reference to volume driver")
	log.G(ctx).WithFields(log.Fields{logrus.ErrorKey: err, "driver": driverName, "volume-name": name}).Error("Error releasing reference to volume driver")

Conversation

thaJeztah commented Oct 16, 2024

Uh oh!

AkihiroSuda Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 17, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 18, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants