-
Notifications
You must be signed in to change notification settings - Fork 249
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
feat_: init logs in InitializeApplication
#6117
Conversation
InitializeApplication
InitializeApplication
Jenkins BuildsClick to see older builds (67)
|
c3909f7
to
f782827
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6117 +/- ##
===========================================
- Coverage 60.98% 60.96% -0.02%
===========================================
Files 827 827
Lines 109734 109764 +30
===========================================
- Hits 66924 66922 -2
- Misses 34970 34992 +22
- Partials 7840 7850 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, LGTM, minor comments left
2102cc2
to
79ef9ac
Compare
5a18c97
to
268277a
Compare
268277a
to
494726d
Compare
Waiting for |
42772eb
to
e30c6aa
Compare
Below is the test passing locally:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a very minor comment/suggestion @igor-sirotin. Thanks for bringing yet another improvement!
accs, err := statusBackend.GetAccounts() | ||
if err != nil { | ||
return makeJSONResponse(err) | ||
} | ||
centralizedMetricsInfo, err := statusBackend.CentralizedMetricsInfo() | ||
|
||
// Read centralized metrics info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code speaks for itself without comment.
@@ -80,38 +97,69 @@ func initializeApplication(requestJSON string) string { | |||
return makeJSONResponse(err) | |||
} | |||
|
|||
// Initialize logs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Untested suggestion, but this log initialization section could be a separate function. Usually catches my attention whenever I see comments describing the what because the more useful comments are about the why and what comments can often times be made irrelevant by good function names and good function composition. Unfortunately in Go, function composition can be cumbersome without some abstractions.
func initLogs(request requests.InitializeApplication) error {
if request.LogDir == "" {
request.LogDir = request.DataDir
}
logSettings := logutils.LogSettings{
Enabled: request.LogEnabled,
Level: request.LogLevel,
File: path.Join(request.LogDir, api.DefaultLogFile),
}
var err error
if err = logutils.OverrideRootLoggerWithConfig(logSettings); err == nil {
logutils.ZapLogger().Info("logging initialised",
zap.Any("logSettings", logSettings),
zap.Bool("APILoggingEnabled", request.APILoggingEnabled),
)
} else {
return err
}
if request.APILoggingEnabled {
logRequestsFile := path.Join(request.LogDir, api.DefaultAPILogFile)
return requestlog.ConfigureAndEnableRequestLogging(logRequestsFile)
}
return nil
}
err = initLogs(request)
if err != nil {
return makeJSONResponse(err)
}
@ilmotta thanks for the review 👍 |
Coverage is there with |
Needed for status-im/status-desktop#16414
Description
Moved logic from
InitLogging
endpoint toInitializeApplication
.No need for a separate endpoint.
InitLogging
is now deprecated.Simplified arguments for logging initialization:
LogDir
(and fallback toDataDir
if empty)LogEnabled
LogLevel
APILoggingEnabled
(renamed fromLogRequestGo
, because we don't just log requests, we also log responses. And might add signals there as well)Log filenames is defined in
status-go
now:geth.log
for main logapi.log
for API logLogSettings.MobileSystem
is now deprecated as redundant, any value will be overridden withfalse
.It was always set to
false
on status-mobile. And was never set instatus-desktop
.Extracted some lower-level functions in
mobile/callog/status_request_log.go
Needed for custom cases like
InitializaApplication
: we enable the API logging in this endpoint, so we need to attempt logging AFTER the call finishes.Added a functional test for this 🎉