Skip to content

Conversation

@dirkmc
Copy link
Contributor

@dirkmc dirkmc commented Aug 24, 2021

No description provided.

@dirkmc dirkmc requested a review from aarshkshah1992 August 24, 2021 09:55
@codecov-commenter
Copy link

Codecov Report

Merging #241 (78441cd) into master (e69ae98) will decrease coverage by 0.00%.
The diff coverage is 81.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #241      +/-   ##
==========================================
- Coverage   67.90%   67.89%   -0.01%     
==========================================
  Files          24       24              
  Lines        3050     3065      +15     
==========================================
+ Hits         2071     2081      +10     
- Misses        624      628       +4     
- Partials      355      356       +1     
Impacted Files Coverage Δ
transport/graphsync/graphsync.go 77.85% <72.72%> (-0.07%) ⬇️
channelmonitor/channelmonitor.go 75.33% <84.00%> (+1.40%) ⬆️
impl/events.go 75.08% <100.00%> (ø)
network/libp2p_impl.go 68.38% <0.00%> (-2.95%) ⬇️

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 60cf299...78441cd. Read the comment docs.

@dirkmc dirkmc merged commit c5b8330 into master Aug 24, 2021
@dirkmc dirkmc deleted the refactor/chanmon-reduce-logging branch August 24, 2021 12:44
mpc.restartChannelDebounced = func() { debouncer(mpc.restartChannel) }
mpc.restartChannelDebounced = func(err error) {
// Log the error at debug level
log.Debug(err.Error())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI:

log.Debugw("what does this mean", "error", err)

Otherwise:

  1. We pay for the overhead of calling Error, even if we don't have debug logging enabled.
  2. The user will see log messages like "stream reset" with no context.

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.

5 participants