fix_: set log level to debug for mobile release build doesn't generate log file #6202
Conversation
Jenkins BuildsClick to see older builds (16)
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/7.1.x #6202 +/- ##
================================================
Coverage ? 60.95%
================================================
Files ? 832
Lines ? 109876
Branches ? 0
================================================
Hits ? 66972
Misses ? 35058
Partials ? 7846
Flags with carried forward coverage won't be shown. Click here to find out more.
|
qfrank
changed the title
|
I'm a bit confused with this fix. |
There was a problem hiding this comment.
I checked the desktop code and we're using this endpoint when toggling Debug.
Lines 1807 to 1809 in 50933aa
Works just fine, I can see the debug logs after restart.
I also understand the confusion, I am also surprised that log_level is stored both 2 tables: settings and node_config. We definitely need to clean this up.
@qfrank @ilmotta Knowing the release urgency, I am not against this fix if it helps.
But it seems that status-mobile should use a different endpoint (not because it's the right one, but because it's the working one). I suggest that to be done at least for the develop cherry-pick.
| func (m *Messenger) setLogLevel(level string, logger *zap.Logger) { | ||
| if level == "" { | ||
| return | ||
| } | ||
| err := nodecfg.SetLogLevel(m.database, level) | ||
| if err != nil { | ||
| logger.Error("nodecfg.SetLogLevel", zap.Error(err)) | ||
| } | ||
| } |
There was a problem hiding this comment.
There is such a method already:
status-go/protocol/messenger_settings.go
Lines 33 to 39 in 50933aa
There was a problem hiding this comment.
I agree frontend should use different endpoint(that was also my consideration when trying to fix this) and we definitely should do it like this later, but based on the release urgency, I prefer fix it with current way.
setLogLevel won't enable/disable log. And it require extra effort on the frontend side. @igor-sirotin
There was a problem hiding this comment.
@qfrank but.. why did log ended up being disabled?
There was a problem hiding this comment.
@qfrank but.. why did log ended up being disabled?
because mobile disabled log with release build by default @igor-sirotin
There was a problem hiding this comment.
also pls see the discussions why we didn't enable it by default ATM @igor-sirotin
There was a problem hiding this comment.
Alright, let's merge it for the release.
But for develop fix we'll need:
- remove code duplication
- probably use a different endpoint from status-mobile
And in general, we need to review the way we store settings and all this mess with NodeConfig. But it's a separate big story.
Currently, mobile release build disabled log by default via
.env.release, so there's no log files generated(it was generated before, that's because when log is disabled with.env, frontend will invokeinitLoggingwith logLevelERRORwhich should be an old bug). However, when user change log level from disabled todebug, then re-login, still no log files(e.g. geth.log) as log is still disabled inlog_config, this PR fixed this issue,relate mobile issue