feat: add separate flag for metrics #1504

AntiTyping · 2024-09-16T20:11:59Z

Refactors debug and metrics servers into two separate servers

adds --enable-metrics-server
uses 127.0.0.1 as the default address for debug and metrics servers
uses port 5184 as the default port for metrics server
adds metrics-listen-addr setting to config.yaml
rly config init will generate the following config
renamed api-listen-addr to debug-listen-addr
renames --debug-addr to --debug-listen-addr to be consistent with config

global:
    debug-listen-addr: 127.0.0.1:5183
    metrics-listen-addr: 127.0.0.1:5184
    ...

manually test the relayer to make sure it works today
add unit tests to make sure it works today and in the future
old --debug-addr should still work but with deprication message

freak12techno · 2024-09-16T20:59:58Z

Just saying, 9100 is a default port for node_exporter, which is widely used everywhere for HW metrics collection, so it might clash.

cmd/flags.go

cmd/config.go

* uses metrics-listen-addr for metrics server * sets default debug server api-listen-addr to 127.0.0.1:5183 * sets default metrics server metrics-listen-addr to 127.0.0.1:9100

* moves metrics to separate server

* removes address flags so config file is the source of truth for addresses * adds tests for flags to enable debug and metrics servers * adds tests for config file settings for addresses for debug and metrics servers * adjusts log messages

* restores address flags * renames `--debug-addr` to `--debug-listen-addr` to be consistent with config file setting `debug-listen-addr:` * renames `--metrics-addr` to `--metrics-listen-addr` to be consistent with config file setting `metrics-listen-addr:` * updates docs

jtieri · 2024-09-27T17:30:50Z

README.md

@@ -193,7 +193,7 @@ Additional information on how IBC works can be found [here](https://ibc.cosmos.n
     $ rly start
     ```

-    >When running multiple instances of `rly start`, you will need to use the `--debug-addr` flag and provide an address:port. You can also pass an empty string `''`  to turn off this feature or pass `localhost:0` to randomly select a port.
+    >When running multiple instances of `rly start`, you will need to use the `--enable-debug-server` and `--debug-listen-addr` flag and provide an address:port. You can also pass an empty string `''`  to turn off this feature or pass `localhost:0` to randomly select a port.


Changes look good, as discussed in our call we will support the old flags for now and mark them deprecated in the CLI output and the docs here.

jtieri · 2024-09-27T17:31:22Z

cmd/config.go

+	DebugListenPort   string `yaml:"debug-listen-addr" json:"debug-listen-addr"`
+	MetricsListenPort string `yaml:"metrics-listen-addr" json:"metrics-listen-addr"`
+	Timeout           string `yaml:"timeout" json:"timeout"`
+	Memo              string `yaml:"memo" json:"memo"`
+	LightCacheSize    int    `yaml:"light-cache-size" json:"light-cache-size"`
+	LogLevel          string `yaml:"log-level" json:"log-level"`
+	ICS20MemoLimit    int    `yaml:"ics20-memo-limit" json:"ics20-memo-limit"`
+	MaxReceiverSize   int    `yaml:"max-receiver-size" json:"max-receiver-size"`


As discussed we will support the old config schema and mark it deprecated for one release

jtieri · 2024-09-27T17:32:05Z

cmd/config_test.go

+	}{
+		{
+			"debug-listen-addr: 127.0.0.1:5183",
+			true,
+		},
+		{
+			"metrics-listen-addr: 127.0.0.1:5184",
+			true,
+		},
+	}


Add test case to support the old config as well until we remove them

jtieri · 2024-09-27T17:32:20Z

cmd/flags.go

+	flagDebugListenAddr                = "debug-listen-addr"
+	flagEnableMetricsServer            = "enable-metrics-server"
+	flagMetricsListenAddr              = "metrics-listen-addr"


jtieri · 2024-09-27T17:32:45Z

cmd/flags.go

+		"address to use for metrics server. By default, "+
+			"will be the metrics-listen-addr parameter in the global config. "+
+			"Make sure to enable metrics server using --enable-metrics-server flag.",
+	)


Make sure we add a comment stating we will deprecate the old flags in next release for the new ones

jtieri · 2024-09-27T17:33:07Z

cmd/start.go

 	cmd = processorFlag(a.viper, cmd)
 	cmd = initBlockFlag(a.viper, cmd)
 	cmd = flushIntervalFlag(a.viper, cmd)
 	cmd = memoFlag(a.viper, cmd)
 	cmd = stuckPacketFlags(a.viper, cmd)
 	return cmd
 }
+
+func setupMetricsServer(cmd *cobra.Command, a *appState, err error, chains map[string]*relayer.Chain) (*processor.PrometheusMetrics, error) {


Great idea to break out the functionality into smaller methods!

jtieri · 2024-09-27T17:33:36Z

cmd/start_test.go

+func TestMetricsServerFlags(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {


Make sure to add test case supporting old flags until we remove them

jtieri · 2024-09-27T17:37:17Z

cmd/start_test.go

+		args          []string
+		newSetting    string
+		wantedPort    int
+		wantedRunning bool


One pattern I see often that I like is to add something like a testName string field to the test table struct where the value is something like this is what the test case is testing - should it pass or fail. This is helpful when looking through stdout or the CI output to see what a test case was supposed to do and if it was supposed to pass/fail

Ex: Run start with flag to enable debug server - Should pass

jtieri · 2024-09-27T17:49:01Z

cmd/start_test.go

+		defer conn.Close()
+	}
+	require.NoError(t, err, "Server should be running")
+	res, err := http.Get(fmt.Sprintf("http://127.0.0.1:%d/debug/pprof/goroutine", port))


one thing to point out is that when you make an http request and get a response it is the callers responsibility to ensure the response body is closed. see: https://pkg.go.dev/net/http

Ex:

defer res.Body.Close()

jtieri · 2024-09-27T17:52:03Z

examples/config_EXAMPLE.yaml

+    debug-listen-addr: 127.0.0.1:5183
+    metrics-listen-addr: 127.0.0.1:5184


As discussed, make sure we support old schema and mark it deprecated.

AntiTyping requested a review from a team as a code owner September 16, 2024 20:12

AntiTyping force-pushed the andy/feat/add-separate-flag-for-metrics branch from 709729a to 17a9b80 Compare September 16, 2024 20:27

Reecepbcups reviewed Sep 17, 2024

View reviewed changes

cmd/flags.go Outdated Show resolved Hide resolved

Reecepbcups changed the title ~~Andy/feat/add separate flag for metrics~~ feat: add separate flag for metrics Sep 17, 2024

jtieri reviewed Sep 17, 2024

View reviewed changes

cmd/flags.go Outdated Show resolved Hide resolved

cmd/config.go Outdated Show resolved Hide resolved

AntiTyping added 4 commits September 18, 2024 16:42

feat: adds metrics server address to config and sets localhost defaults

0665664

* uses metrics-listen-addr for metrics server * sets default debug server api-listen-addr to 127.0.0.1:5183 * sets default metrics server metrics-listen-addr to 127.0.0.1:9100

refactor: separates debug and metrics server

cd48431

* moves metrics to separate server

feat: changes the default metrics port to 5184

ae03f4e

AntiTyping force-pushed the andy/feat/add-separate-flag-for-metrics branch 2 times, most recently from f2318fa to 6edcf5c Compare September 19, 2024 15:36

refactor: renames api-listen-addr to debug-listen-addr

ec7bb41

AntiTyping force-pushed the andy/feat/add-separate-flag-for-metrics branch from 6edcf5c to ec7bb41 Compare September 19, 2024 15:58

AntiTyping added 3 commits September 19, 2024 13:52

feat: improves error reporting

767bd86

chore: code cleanup

1142eef

jtieri reviewed Sep 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add separate flag for metrics #1504

feat: add separate flag for metrics #1504

AntiTyping commented Sep 16, 2024 •

edited

Loading

freak12techno commented Sep 16, 2024

jtieri Sep 27, 2024

jtieri Sep 27, 2024

jtieri Sep 27, 2024

jtieri Sep 27, 2024

jtieri Sep 27, 2024

jtieri Sep 27, 2024

jtieri Sep 27, 2024

jtieri Sep 27, 2024

jtieri Sep 27, 2024

jtieri Sep 27, 2024

		debug-listen-addr: 127.0.0.1:5183
		metrics-listen-addr: 127.0.0.1:5184

feat: add separate flag for metrics #1504

Are you sure you want to change the base?

feat: add separate flag for metrics #1504

Conversation

AntiTyping commented Sep 16, 2024 • edited Loading

freak12techno commented Sep 16, 2024

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

AntiTyping commented Sep 16, 2024 •

edited

Loading