Be consistent in logging errors and use `zap.Error` #1502

New issue

Closed

opened 2024-11-18 08:55:33 +00:00 by a-savchuk · 4 comments

a-savchuk commented

2024-11-18 08:55:33 +00:00

Member

Here and there, I see zap.String("error", err.Error()) or zap.String("err", err.Error()), and that makes me sad

$ grep --include=*.go -rE 'zap.String\("err' | head
cmd/frostfs-ir/httpcomponent.go:                                zap.String("error", err.Error()),
cmd/frostfs-ir/main.go:                 zap.String("error", err.Error()),
cmd/frostfs-ir/main.go:                 zap.String("error", err.Error()),
cmd/frostfs-ir/pprof.go:                                zap.String("error", err.Error()))
cmd/frostfs-node/config.go:                             zap.String("error", err.Error()),
cmd/frostfs-node/config.go:                     zap.String("error", err.Error()))
cmd/frostfs-node/main.go:                       zap.String("error", err.Error()),
cmd/frostfs-node/morph.go:                      zap.String("error", err.Error()),
cmd/frostfs-node/morph.go:              c.log.Warn(ctx, logs.FrostFSNodeCantGetLastProcessedSideChainBlockNumber, zap.String("error", err.Error()))
cmd/frostfs-node/netmap.go:                                     zap.String("error", err.Error()),

$ grep --include=*.go -rE 'zap.String\("err' | wc -l
114

Here and there, I see `zap.String("error", err.Error())` or `zap.String("err", err.Error())`, and that makes me sad ``` $ grep --include=*.go -rE 'zap.String\("err' | head cmd/frostfs-ir/httpcomponent.go: zap.String("error", err.Error()), cmd/frostfs-ir/main.go: zap.String("error", err.Error()), cmd/frostfs-ir/main.go: zap.String("error", err.Error()), cmd/frostfs-ir/pprof.go: zap.String("error", err.Error())) cmd/frostfs-node/config.go: zap.String("error", err.Error()), cmd/frostfs-node/config.go: zap.String("error", err.Error())) cmd/frostfs-node/main.go: zap.String("error", err.Error()), cmd/frostfs-node/morph.go: zap.String("error", err.Error()), cmd/frostfs-node/morph.go: c.log.Warn(ctx, logs.FrostFSNodeCantGetLastProcessedSideChainBlockNumber, zap.String("error", err.Error())) cmd/frostfs-node/netmap.go: zap.String("error", err.Error()), ``` ``` $ grep --include=*.go -rE 'zap.String\("err' | wc -l 114 ```

a-savchuk added the

triage

observability

labels 2024-11-18 08:55:33 +00:00

fyrchik commented

2024-11-18 10:28:12 +00:00

Owner

IIRC, the reason for that was that zap.Error printed a stacktrace, and we would like to be less verbose.
No other problems.

IIRC, the reason for that was that `zap.Error` printed a stacktrace, and we would like to be less verbose. No other problems.

👍 1

a-savchuk commented

2024-11-18 10:55:49 +00:00

Author

Member

@fyrchik So, should I close it?

dstepanov-yadro commented

2024-11-18 14:03:43 +00:00

Member

According to the issue on github, initially the problem was with errors from the pkg/errors, which stored information about the stack inside: https://github.com/pkg/errors/blob/master/errors.go#L105

Errors from stdlib do not store the stack trace, so I agree that it is possible to replace all zap.String("error", err.Error()) calls with zap.Error(err)

According to the [issue on github](https://github.com/uber-go/zap/issues/650), initially the problem was with errors from the [pkg/errors](https://github.com/pkg/errors), which stored information about the stack inside: https://github.com/pkg/errors/blob/master/errors.go#L105 Errors from stdlib do not store the stack trace, so I agree that it is possible to replace all `zap.String("error", err.Error())` calls with `zap.Error(err)`

👍 1