From 7ef8e043f87f1dfd2e0dcd81556b1b80ce3a4f63 Mon Sep 17 00:00:00 2001 From: George Kudrayvtsev Date: Wed, 17 Mar 2021 16:57:55 -0700 Subject: [PATCH 1/6] Introduce the --captive-core-log-path parameter: Now, we either log to Horizon's subservice=stellar-core or we pass along to Stellar-Core's LOG_FILE_PATH="..." based on the value of the above parameter. --- ingest/ledgerbackend/captive_core_backend.go | 2 ++ ingest/ledgerbackend/stellar_core_runner.go | 5 ++++- services/horizon/internal/config.go | 1 + services/horizon/internal/flags.go | 6 ++++++ services/horizon/internal/ingest/main.go | 14 +++++++++++++- services/horizon/internal/init.go | 1 + 6 files changed, 27 insertions(+), 2 deletions(-) diff --git a/ingest/ledgerbackend/captive_core_backend.go b/ingest/ledgerbackend/captive_core_backend.go index 10f7c29db5..c7904454ad 100644 --- a/ingest/ledgerbackend/captive_core_backend.go +++ b/ingest/ledgerbackend/captive_core_backend.go @@ -111,6 +111,8 @@ type CaptiveCoreConfig struct { NetworkPassphrase string // HistoryArchiveURLs are a list of history archive urls HistoryArchiveURLs []string + // LogPath is the path in which to store Core logs + LogPath string // Optional fields diff --git a/ingest/ledgerbackend/stellar_core_runner.go b/ingest/ledgerbackend/stellar_core_runner.go index 72a7f7064d..ff9826c3b3 100644 --- a/ingest/ledgerbackend/stellar_core_runner.go +++ b/ingest/ledgerbackend/stellar_core_runner.go @@ -48,6 +48,7 @@ type pipe struct { } type stellarCoreRunner struct { + logPath string executablePath string configAppendPath string networkPassphrase string @@ -82,6 +83,7 @@ func newStellarCoreRunner(config CaptiveCoreConfig, mode stellarCoreRunnerMode) ctx, cancel := context.WithCancel(config.Context) runner := &stellarCoreRunner{ + logPath: config.LogPath, executablePath: config.BinaryPath, configAppendPath: config.ConfigAppendPath, networkPassphrase: config.NetworkPassphrase, @@ -116,9 +118,10 @@ func (r *stellarCoreRunner) generateConfig() (string, error) { fmt.Sprintf(`NETWORK_PASSPHRASE="%s"`, r.networkPassphrase), fmt.Sprintf(`BUCKET_DIR_PATH="%s"`, filepath.Join(r.tempDir, "buckets")), fmt.Sprintf(`HTTP_PORT=%d`, r.httpPort), - `LOG_FILE_PATH=""`, } + lines = append(lines, fmt.Sprintf(`LOG_FILE_PATH="%s"`, r.logPath)) + if r.mode == stellarCoreRunnerModeOffline { // In offline mode, there is no need to connect to peers lines = append(lines, "RUN_STANDALONE=true") diff --git a/services/horizon/internal/config.go b/services/horizon/internal/config.go index 9c55a85cac..331f2bc444 100644 --- a/services/horizon/internal/config.go +++ b/services/horizon/internal/config.go @@ -21,6 +21,7 @@ type Config struct { CaptiveCoreConfigAppendPath string RemoteCaptiveCoreURL string CaptiveCoreHTTPPort uint + CaptiveCoreLogPath string StellarCoreDatabaseURL string StellarCoreURL string diff --git a/services/horizon/internal/flags.go b/services/horizon/internal/flags.go index 6e9c23960b..ee80f5c6e6 100644 --- a/services/horizon/internal/flags.go +++ b/services/horizon/internal/flags.go @@ -255,6 +255,12 @@ func Flags() (*Config, support.ConfigOptions) { OptType: types.String, Usage: "name of the file where logs will be saved (leave empty to send logs to stdout)", }, + &support.ConfigOption{ + Name: "captive-core-log-path", + ConfigKey: &config.CaptiveCoreLogPath, + OptType: types.String, + Usage: "name of the path for Core logs (leave empty to log w/ Horizon)", + }, &support.ConfigOption{ Name: "max-path-length", ConfigKey: &config.MaxPathLength, diff --git a/services/horizon/internal/ingest/main.go b/services/horizon/internal/ingest/main.go index 762295e905..69ebab52af 100644 --- a/services/horizon/internal/ingest/main.go +++ b/services/horizon/internal/ingest/main.go @@ -71,6 +71,7 @@ type Config struct { CaptiveCoreBinaryPath string CaptiveCoreConfigAppendPath string CaptiveCoreHTTPPort uint + CaptiveCoreLogPath string RemoteCaptiveCoreURL string NetworkPassphrase string @@ -189,8 +190,19 @@ func NewSystem(config Config) (System, error) { return nil, errors.Wrap(err, "error creating captive core backend") } } else { + // If the user wants a custom logging location for Captive Core (set + // via Core's LOG_FILE_PATH), it takes priority over Horizon's + // internal logging. + // + // https://github.com/stellar/go/issues/3438#issuecomment-797690998 + var logger *logpkg.Entry = nil + if config.CaptiveCoreLogPath == "" { + logger = log.WithField("subservice", "stellar-core") + } + ledgerBackend, err = ledgerbackend.NewCaptive( ledgerbackend.CaptiveCoreConfig{ + LogPath: config.CaptiveCoreLogPath, BinaryPath: config.CaptiveCoreBinaryPath, ConfigAppendPath: config.CaptiveCoreConfigAppendPath, HTTPPort: config.CaptiveCoreHTTPPort, @@ -198,7 +210,7 @@ func NewSystem(config Config) (System, error) { HistoryArchiveURLs: []string{config.HistoryArchiveURL}, CheckpointFrequency: config.CheckpointFrequency, LedgerHashStore: ledgerbackend.NewHorizonDBLedgerHashStore(config.HistorySession), - Log: log.WithField("subservice", "stellar-core"), + Log: logger, Context: ctx, }, ) diff --git a/services/horizon/internal/init.go b/services/horizon/internal/init.go index 0108e28ef4..90677d16a5 100644 --- a/services/horizon/internal/init.go +++ b/services/horizon/internal/init.go @@ -72,6 +72,7 @@ func initIngester(app *App) { CaptiveCoreBinaryPath: app.config.CaptiveCoreBinaryPath, CaptiveCoreConfigAppendPath: app.config.CaptiveCoreConfigAppendPath, CaptiveCoreHTTPPort: app.config.CaptiveCoreHTTPPort, + CaptiveCoreLogPath: app.config.CaptiveCoreLogPath, RemoteCaptiveCoreURL: app.config.RemoteCaptiveCoreURL, EnableCaptiveCore: app.config.EnableCaptiveCoreIngestion, DisableStateVerification: app.config.IngestDisableStateVerification, From 819c9e38b61998e7a8cd1d26abfdddfe62b84d9f Mon Sep 17 00:00:00 2001 From: George Kudrayvtsev Date: Wed, 17 Mar 2021 17:52:24 -0700 Subject: [PATCH 2/6] Prevent internal logging entirely if a custom log path is set --- ingest/ledgerbackend/captive_core_backend.go | 15 +++++++++++++-- ingest/ledgerbackend/stellar_core_runner.go | 3 +-- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/ingest/ledgerbackend/captive_core_backend.go b/ingest/ledgerbackend/captive_core_backend.go index c7904454ad..f832295098 100644 --- a/ingest/ledgerbackend/captive_core_backend.go +++ b/ingest/ledgerbackend/captive_core_backend.go @@ -3,6 +3,7 @@ package ledgerbackend import ( "context" "encoding/hex" + "io/ioutil" "os" "sync" @@ -136,11 +137,21 @@ type CaptiveCoreConfig struct { func NewCaptive(config CaptiveCoreConfig) (*CaptiveStellarCore, error) { // Here we set defaults in the config. Because config is not a pointer this code should // not mutate the original CaptiveCoreConfig instance which was passed into NewCaptive() + + // Create a default logging instance for Captive Core's output if no other + // logging configuration was specified. if config.Log == nil { config.Log = log.New() - config.Log.Logger.SetOutput(os.Stdout) - config.Log.SetLevel(logrus.InfoLevel) + if config.LogPath != "" { + // We still create an empty logger instance so there's no nil + // pointer floating around beyond this call. + config.Log.Logger.SetOutput(ioutil.Discard) + } else { + config.Log.Logger.SetOutput(os.Stdout) + config.Log.SetLevel(logrus.InfoLevel) + } } + parentCtx := config.Context if parentCtx == nil { parentCtx = context.Background() diff --git a/ingest/ledgerbackend/stellar_core_runner.go b/ingest/ledgerbackend/stellar_core_runner.go index ff9826c3b3..9889322fb2 100644 --- a/ingest/ledgerbackend/stellar_core_runner.go +++ b/ingest/ledgerbackend/stellar_core_runner.go @@ -118,10 +118,9 @@ func (r *stellarCoreRunner) generateConfig() (string, error) { fmt.Sprintf(`NETWORK_PASSPHRASE="%s"`, r.networkPassphrase), fmt.Sprintf(`BUCKET_DIR_PATH="%s"`, filepath.Join(r.tempDir, "buckets")), fmt.Sprintf(`HTTP_PORT=%d`, r.httpPort), + fmt.Sprintf(`LOG_FILE_PATH="%s"`, r.logPath), } - lines = append(lines, fmt.Sprintf(`LOG_FILE_PATH="%s"`, r.logPath)) - if r.mode == stellarCoreRunnerModeOffline { // In offline mode, there is no need to connect to peers lines = append(lines, "RUN_STANDALONE=true") From 6ce55527424a27725d3d9ea6dfbb70de0b18b53a Mon Sep 17 00:00:00 2001 From: George Kudrayvtsev Date: Thu, 18 Mar 2021 14:07:19 -0700 Subject: [PATCH 3/6] Update docstrings and move the parameter accordingly --- ingest/ledgerbackend/captive_core_backend.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ingest/ledgerbackend/captive_core_backend.go b/ingest/ledgerbackend/captive_core_backend.go index f832295098..783137f4e2 100644 --- a/ingest/ledgerbackend/captive_core_backend.go +++ b/ingest/ledgerbackend/captive_core_backend.go @@ -112,8 +112,6 @@ type CaptiveCoreConfig struct { NetworkPassphrase string // HistoryArchiveURLs are a list of history archive urls HistoryArchiveURLs []string - // LogPath is the path in which to store Core logs - LogPath string // Optional fields @@ -126,7 +124,11 @@ type CaptiveCoreConfig struct { HTTPPort uint // Log is an (optional) custom logger which will capture any output from the Stellar Core process. // If Log is omitted then all output will be printed to stdout. + // However, if LogPath (optional, below) is set, no logging will be done to this. Log *log.Entry + // LogPath is the (optional) path in which to store Core logs, passed along + // to Stellar-Core's LOG_FILE_PATH + LogPath string // Context is the (optional) context which controls the lifetime of a CaptiveStellarCore instance. Once the context is done // the CaptiveStellarCore instance will not be able to stream ledgers from Stellar Core or spawn new // instances of Stellar Core. If Context is omitted CaptiveStellarCore will default to using context.Background. From 8d4aa56b6085142d03090b58c7f00762cecf750a Mon Sep 17 00:00:00 2001 From: George Kudrayvtsev Date: Thu, 18 Mar 2021 14:43:29 -0700 Subject: [PATCH 4/6] Always log Captive Core to Horizon, rather than only if LOG_FILE_PATH is unset --- ingest/ledgerbackend/captive_core_backend.go | 14 +++----------- services/horizon/internal/ingest/main.go | 11 +---------- 2 files changed, 4 insertions(+), 21 deletions(-) diff --git a/ingest/ledgerbackend/captive_core_backend.go b/ingest/ledgerbackend/captive_core_backend.go index 783137f4e2..75ed0ec494 100644 --- a/ingest/ledgerbackend/captive_core_backend.go +++ b/ingest/ledgerbackend/captive_core_backend.go @@ -3,7 +3,6 @@ package ledgerbackend import ( "context" "encoding/hex" - "io/ioutil" "os" "sync" @@ -140,18 +139,11 @@ func NewCaptive(config CaptiveCoreConfig) (*CaptiveStellarCore, error) { // Here we set defaults in the config. Because config is not a pointer this code should // not mutate the original CaptiveCoreConfig instance which was passed into NewCaptive() - // Create a default logging instance for Captive Core's output if no other - // logging configuration was specified. + // Log Captive Core straight to stdout by default if config.Log == nil { config.Log = log.New() - if config.LogPath != "" { - // We still create an empty logger instance so there's no nil - // pointer floating around beyond this call. - config.Log.Logger.SetOutput(ioutil.Discard) - } else { - config.Log.Logger.SetOutput(os.Stdout) - config.Log.SetLevel(logrus.InfoLevel) - } + config.Log.Logger.SetOutput(os.Stdout) + config.Log.SetLevel(logrus.InfoLevel) } parentCtx := config.Context diff --git a/services/horizon/internal/ingest/main.go b/services/horizon/internal/ingest/main.go index 69ebab52af..14690ffc24 100644 --- a/services/horizon/internal/ingest/main.go +++ b/services/horizon/internal/ingest/main.go @@ -190,16 +190,7 @@ func NewSystem(config Config) (System, error) { return nil, errors.Wrap(err, "error creating captive core backend") } } else { - // If the user wants a custom logging location for Captive Core (set - // via Core's LOG_FILE_PATH), it takes priority over Horizon's - // internal logging. - // - // https://github.com/stellar/go/issues/3438#issuecomment-797690998 - var logger *logpkg.Entry = nil - if config.CaptiveCoreLogPath == "" { - logger = log.WithField("subservice", "stellar-core") - } - + logger := log.WithField("subservice", "stellar-core") ledgerBackend, err = ledgerbackend.NewCaptive( ledgerbackend.CaptiveCoreConfig{ LogPath: config.CaptiveCoreLogPath, From 96ba4bddad552456cda259d9612af8139a2a9cbf Mon Sep 17 00:00:00 2001 From: George Kudrayvtsev Date: Thu, 18 Mar 2021 14:45:17 -0700 Subject: [PATCH 5/6] Update docstrings to reflect changes --- ingest/ledgerbackend/captive_core_backend.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ingest/ledgerbackend/captive_core_backend.go b/ingest/ledgerbackend/captive_core_backend.go index 75ed0ec494..07cf53ea62 100644 --- a/ingest/ledgerbackend/captive_core_backend.go +++ b/ingest/ledgerbackend/captive_core_backend.go @@ -123,10 +123,9 @@ type CaptiveCoreConfig struct { HTTPPort uint // Log is an (optional) custom logger which will capture any output from the Stellar Core process. // If Log is omitted then all output will be printed to stdout. - // However, if LogPath (optional, below) is set, no logging will be done to this. Log *log.Entry // LogPath is the (optional) path in which to store Core logs, passed along - // to Stellar-Core's LOG_FILE_PATH + // to Stellar Core's LOG_FILE_PATH LogPath string // Context is the (optional) context which controls the lifetime of a CaptiveStellarCore instance. Once the context is done // the CaptiveStellarCore instance will not be able to stream ledgers from Stellar Core or spawn new From bfce05c6ff1a79c7179cc119d8e49a5c963703b5 Mon Sep 17 00:00:00 2001 From: George Date: Fri, 19 Mar 2021 11:54:09 -0700 Subject: [PATCH 6/6] Clarify behavior in command-line --help text Co-authored-by: Bartek Nowotarski --- services/horizon/internal/flags.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/horizon/internal/flags.go b/services/horizon/internal/flags.go index ee80f5c6e6..964f5eaea4 100644 --- a/services/horizon/internal/flags.go +++ b/services/horizon/internal/flags.go @@ -259,7 +259,7 @@ func Flags() (*Config, support.ConfigOptions) { Name: "captive-core-log-path", ConfigKey: &config.CaptiveCoreLogPath, OptType: types.String, - Usage: "name of the path for Core logs (leave empty to log w/ Horizon)", + Usage: "name of the path for Core logs (leave empty to log w/ Horizon only)", }, &support.ConfigOption{ Name: "max-path-length",