Logging improvement #13

Jason-Zhangxin-Chen · 2023-12-18T06:12:37Z

To close #10, this PR contains:

logging improvements in oracle server and plugins.
better error handling in plugins.

vpiyush · 2023-12-18T12:06:33Z

config/config.go

@@ -24,23 +24,30 @@ var (
 )

 const Version = "v0.1.5"
+const UsageOracleKey = "Set the oracle server key file path."
+const UsagePluginConf = "Set the plugins' configuration file path."


type plugins' ==> plugin's

type plugins' ==> plugin's

Resolved in bd07764

vpiyush · 2023-12-18T12:09:09Z

oracle_server/oracle_server.go

@@ -391,7 +392,7 @@ func (os *OracleServer) reportWithCommitment(newRound uint64, lastRoundData *typ
 		return err
 	}

-	os.logger.Info("oracle client left fund", "balance", balance.String())
+	os.logger.Info("oracle server account left fund", "address", os.key.Address, "balance", balance.String())


"left fund" can be removed from the log. Or say remaining balance. there is a balance field getting printed sperately.

"left fund" can be removed from the log. Or say remaining balance. there is a balance field getting printed sperately.

Resolved in bd07764

vpiyush · 2023-12-18T12:10:03Z

oracle_server/oracle_server.go

@@ -407,7 +408,7 @@ func (os *OracleServer) reportWithoutCommitment(lastRoundData *types.RoundData)
 		os.logger.Error("do report", "error", err.Error())
 		return err
 	}
-	os.logger.Info("report without commitment", "TX hash", tx.Hash(), "Nonce", tx.Nonce())
+	os.logger.Info("reported last round data and without current round commitment", "TX hash", tx.Hash(), "Nonce", tx.Nonce())


Should this be info, Or Debug?

Should this be info, Or Debug?

It could be info, because this is the last report of an oracle node who is no longer in the committee due to the epoch rotation.

vpiyush · 2023-12-18T12:13:14Z

oracle_server/oracle_server.go

@@ -472,7 +473,7 @@ func (os *OracleServer) buildRoundData(round uint64) (*types.RoundData, error) {
 	for _, s := range os.protocolSymbols {
 		p, err := os.aggregatePrice(s, int64(os.curSampleTS))
 		if err != nil {
-			os.logger.Error("aggregatePrice", "error", err.Error(), "symbol", s)
+			os.logger.Warn("aggregatePrice", "error", err.Error(), "symbol", s)


If this is a warning. Maybe we should not say error in the message itself. 'price aggregation failed due to "err.Error()"'. Also it should only be warning if there is something to fix/actionable item from end-user point of view. Else this can be a debug

If this is a warning. Maybe we should not say error in the message itself. 'price aggregation failed due to "err.Error()"'. Also it should only be warning if there is something to fix/actionable item from end-user point of view. Else this can be a debug

Resolved in bd07764

vpiyush · 2023-12-18T12:15:52Z

oracle_server/oracle_server.go

@@ -691,18 +687,17 @@ func (os *OracleServer) tryLoadingNewPlugin(f fs.FileInfo, plugConf types.Plugin
 			return
 		}

+		os.logger.Info("** Replacing legacy plugin with new one: ", f.Name(), f.Mode().String())


Why do we use ** in the log? Is it to put more emphasis on this specific log statement?

Why do we use ** in the log? Is it to put more emphasis on this specific log statement?

Yes, it put more emphasis on the testing phase, since this feature is stable, I will remove the stars.

Why do we use ** in the log? Is it to put more emphasis on this specific log statement?

Resolved in bd07764

vpiyush · 2023-12-18T12:18:49Z

plugin_wrapper/plugin_wrapper.go

 		return err
 	}

-	if len(report.BadSymbols) != 0 {
-		pw.logger.Warn("find bad symbols: ", report.BadSymbols)
+	if len(report.UnRecognizeSymbols) != 0 {


i think the correct word would be unrecognizable

i think the correct word would be unrecognizable

Resolved in bd07764

vpiyush · 2023-12-18T12:22:24Z

oracle_server/oracle_server.go

 		pluginWrapper := pWrapper.NewPluginWrapper(os.loggingLevel, f.Name(), os.pluginDIR, os)
 		if err := pluginWrapper.Initialize(); err != nil {
-			os.logger.Error("** Cannot initialize plugin", "name", f.Name(), "error", err.Error())
+			os.logger.Error("Cannot initialize plugin", "name", f.Name(), "error", err.Error())


I think the first Capital letter is not consistent with other logs and also not the preferred logging style in most golang projects. logging is usually the lowercase. Consistent casing is useful while filtering logs

I think the first Capital letter is not consistent with other logs and also not the preferred logging style in most golang projects. logging is usually the lowercase. Consistent casing is useful while filtering logs

Resolved in bd07764

vpiyush · 2023-12-18T12:22:55Z

oracle_server/oracle_server.go

@@ -714,11 +709,11 @@ func (os *OracleServer) ApplyPluginConf(name string, plugConf types.PluginConfig
 	// set the plugin configuration via system env, thus the plugin can load it on startup.
 	conf, err := json.Marshal(plugConf)
 	if err != nil {
-		os.logger.Error("** Cannot marshal plugin's configuration", "error", err.Error())
+		os.logger.Error("Cannot marshal plugin's configuration", "error", err.Error())


same here, other places too

same here, other places too

Resolved in bd07764

vpiyush · 2023-12-18T12:33:22Z

plugins/binance/binance.go

@@ -35,12 +34,12 @@ type BIClient struct {
 func NewBIClient(conf *types.PluginConfig) *BIClient {
 	client := common.NewClient(conf.Key, time.Second*time.Duration(conf.Timeout), conf.Endpoint)
 	if client == nil {


In what cases, is this possible?

In what cases, is this possible?

I doubled checked, it is not possible.
Resolved in bd07764

Jason-Zhangxin-Chen added 3 commits December 14, 2023 13:32

improvement, made logging improvements.

cb0b447

improvement, loggings and code refine.

2aeafaf

improvement, better logging and error handling in plugins.

e38be63

Jason-Zhangxin-Chen requested review from m4ksio, yazzaoui, vpiyush, lorenzo-dev1, cmjc, spenot09, tbssajal and raj-shekhar1 December 18, 2023 06:12

vpiyush reviewed Dec 18, 2023

View reviewed changes

logging, logging improvements.

bd07764

Jason-Zhangxin-Chen requested a review from vpiyush December 21, 2023 06:54

Jason-Zhangxin-Chen merged commit 90e8d2e into master Jan 3, 2024
4 checks passed

cmjc mentioned this pull request Feb 11, 2024

Updates for Sumida protocol upgrade autonity/docs.autonity.org#147

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Logging improvement #13

Logging improvement #13

Jason-Zhangxin-Chen commented Dec 18, 2023

vpiyush Dec 18, 2023

Jason-Zhangxin-Chen Dec 21, 2023

vpiyush Dec 18, 2023

Jason-Zhangxin-Chen Dec 21, 2023

vpiyush Dec 18, 2023

Jason-Zhangxin-Chen Dec 21, 2023

vpiyush Dec 18, 2023

Jason-Zhangxin-Chen Dec 21, 2023

vpiyush Dec 18, 2023

Jason-Zhangxin-Chen Dec 21, 2023

Jason-Zhangxin-Chen Dec 21, 2023

vpiyush Dec 18, 2023

Jason-Zhangxin-Chen Dec 21, 2023

vpiyush Dec 18, 2023

Jason-Zhangxin-Chen Dec 21, 2023

vpiyush Dec 18, 2023

Jason-Zhangxin-Chen Dec 21, 2023

vpiyush Dec 18, 2023

Jason-Zhangxin-Chen Dec 21, 2023 •

edited

Loading

Logging improvement #13

Logging improvement #13

Conversation

Jason-Zhangxin-Chen commented Dec 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jason-Zhangxin-Chen Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

Jason-Zhangxin-Chen Dec 21, 2023 •

edited

Loading