Added processDebugger component #4288
Conversation
iulianpascalau
commented
Jul 12, 2022
- added processDebugger + integration
| processDebugger, err := createDisabledProcessDebugger() | ||
| if err != nil { |
There was a problem hiding this comment.
do we want to use a disabled instance, or this is just at first?
There was a problem hiding this comment.
always use a disable instance in order to prevent all checks for nils. This is the power of the polymorhism :)
| type ProcessDebugConfig struct { | ||
| Enabled bool | ||
| LogLevelChanger string | ||
| GoRoutinesDump bool |
There was a problem hiding this comment.
variables with the same type should stay together (optime memory usage)
GoRoutinesDump can be moved near Enabled field
debug/process/debugger.go
Outdated
| "github.com/ElrondNetwork/elrond-go/config" | ||
| ) | ||
|
|
||
| const minAcceptedValue = 1 |
There was a problem hiding this comment.
maybe you can group the consts together
const (
minAcceptedValue = 1
buffSize = 100 * 1024 * 1024 // 100MB
)
cmd/node/config/config.toml
Outdated
| @@ -800,6 +800,11 @@ | |||
| [Debug.EpochStart] | |||
| GoRoutineAnalyserEnabled = true | |||
| ProcessDataTrieOnCommitEpoch = true | |||
| [Debug.Process] | |||
| Enabled = true | |||
| LogLevelChanger = "*:DEBUG,p2p:TRACE,debug:DEBUG,process:TRACE,intercept:TRACE" | |||
There was a problem hiding this comment.
LogLevelChanger I think this can be renamed
---> DebuggingLogLevel or NewLogLevel
debug/process/debugger.go
Outdated
| } | ||
| d.logChangeHandler = func() { | ||
| errSetLogLevel := logger.SetLogLevel(d.logLevel) | ||
| log.LogIfError(errSetLogLevel) |
There was a problem hiding this comment.
log.LogIfError(errSetLogLevel, "d.logChangeHandle: cannot change log level") -- in order to know where the error appears
debug/process/debugger.go
Outdated
|
|
||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| d.cancel = cancel | ||
| d.goRoutinesDumpHandler = func() { |
There was a problem hiding this comment.
why does this function have to be declared?
it can be expressed as a normal function.
There was a problem hiding this comment.
it was done like this in order to thoroughly test the component (in tests, I swapped these function pointers with testing functions)
There was a problem hiding this comment.
created method
debug/process/debugger.go
Outdated
| numBytes := runtime.Stack(buff, true) | ||
| log.Debug(string(buff[:numBytes])) | ||
| } | ||
| d.logChangeHandler = func() { |
There was a problem hiding this comment.
also, logChangeHandler can be declared
func (debugger *processDebugger) logChangeHandler() {
errSetLogLevel := logger.SetLogLevel(debugger.logLevel)
log.LogIfError(errSetLogLevel)
}
There was a problem hiding this comment.
right, will dump the anonymous functions usage
| @@ -25,7 +27,7 @@ type processDebugger struct { | |||
| goRoutinesDumpHandler func() | |||
There was a problem hiding this comment.
goRoutinesDumpHandler and logChangeHandler can be removed and call the functions directly?
There was a problem hiding this comment.
no, can not properly test the component otherwise
The base branch was changed.
# Conflicts: # factory/interface.go # process/block/baseProcess.go # process/block/metablock.go # process/block/preprocess/gasComputation.go # process/block/shardblock.go # process/errors.go # process/interface.go
Codecov ReportBase: 73.84% // Head: 73.85% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## rc/v1.4.0 #4288 +/- ##
=============================================
+ Coverage 73.84% 73.85% +0.01%
=============================================
Files 689 692 +3
Lines 88127 88257 +130
=============================================
+ Hits 65075 65186 +111
- Misses 18140 18156 +16
- Partials 4912 4915 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |