Skip to content
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

Change logging output to os.Stderr #213

Merged
merged 2 commits into from May 29, 2020
Merged

Conversation

gm42
Copy link

@gm42 gm42 commented May 14, 2020

Which problem is this PR solving?

It is not safe to write directly to os.Stdout/os.Stderr in concurrent scenarios; writes can intermingle and cause corrupt log lines output. See also: https://stackoverflow.com/a/14694666

It turns out it is safe instead, see:

(No issue created yet)

Short description of the changes

Switch logging output to os.Stderr; this is the usual file descriptor for log output, also used by Go's log package: https://golang.org/src/log/log.go?s=9636:9664#L76

@codecov
Copy link

codecov bot commented May 14, 2020

Codecov Report

Merging #213 into master will decrease coverage by 10.26%.
The diff coverage is 51.93%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #213       +/-   ##
===========================================
- Coverage   84.48%   74.22%   -10.27%     
===========================================
  Files          52       56        +4     
  Lines        3074     3212      +138     
===========================================
- Hits         2597     2384      -213     
- Misses        398      764      +366     
+ Partials       79       64       -15     
Impacted Files Coverage Δ
client/amqp/amqp.go 40.98% <ø> (ø)
client/amqp/option.go 66.66% <ø> (ø)
client/amqp/user.pb.go 26.92% <ø> (ø)
client/http/option.go 100.00% <ø> (ø)
client/kafka/async_producer.go 0.00% <0.00%> (ø)
client/kafka/sync_producer.go 0.00% <0.00%> (ø)
client/redis/redis.go 44.44% <ø> (ø)
client/sns/message.go 97.53% <ø> (ø)
client/sns/publisher.go 93.33% <ø> (ø)
client/sql/sql.go 17.53% <ø> (-68.73%) ⬇️
... and 71 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c6c55a...ccf37d7. Read the comment docs.

log/zerolog/factory.go Outdated Show resolved Hide resolved
Signed-off-by: Giuseppe Mazzotta <g.mazzotta@thebeat.co>
@gm42 gm42 changed the title Change logging output to os.Stderr and wrap in a SyncWriter Change logging output to os.Stderr May 25, 2020
@mantzas mantzas self-requested a review May 25, 2020 12:51
@mantzas mantzas merged commit 042c702 into beatlabs:master May 29, 2020
Stefos pushed a commit to Stefos/patron that referenced this pull request Oct 8, 2020
Signed-off-by: Giuseppe Mazzotta <g.mazzotta@thebeat.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants