Fix RPC name override

[dhaidashenko]

No description provided.

[github-actions]

I see that you haven't updated any CHANGELOG files. Would it make sense to do so?

[jmank88]

It is safe to pass the nodeName directly. We validate that all nodes are named. Notice that we already pas it without nil check on the line below.

Suggested change

	name := fmt.Sprintf("eth-sendonly-rpc-%d", i)
	if node.Name != nil && *node.Name != "" {
	name = *node.Name
	}
	rpc := evmclient.NewRPCClient(lggr, empty, (*url.URL)(node.HTTPURL), name, int32(i), chainID,
	commonclient.Secondary)
	rpc := evmclient.NewRPCClient(lggr, empty, (*url.URL)(node.HTTPURL), *node.Name, int32(i), chainID,
	commonclient.Secondary)

[samsondav]

Can we get a test demonstrating what this fixes?

[jmank88]

Can we get a test demonstrating what this fixes?

We haven't had the ability to test prom metrics coherently in the past, since they are all mixed up in global state. However, we could add a txtar test and checks the /metrics endpoint, like for /health: https://github.com/smartcontractkit/chainlink/blob/develop/testdata/scripts/health/multi-chain.txtar

[dhaidashenko]

Can we get a test demonstrating what this fixes?

We haven't had the ability to test prom metrics coherently in the past, since they are all mixed up in global state. However, we could add a txtar test and checks the /metrics endpoint, like for /health: https://github.com/smartcontractkit/chainlink/blob/develop/testdata/scripts/health/multi-chain.txtar

Added the test, but it does not feel that this is the right approach. The script seems too complex for txtar test. Also it's quite limited in terms of available metrics as node does not perform any actions. Automated check against soaked node might be a better option.

[jmank88]

Can we get a test demonstrating what this fixes?

We haven't had the ability to test prom metrics coherently in the past, since they are all mixed up in global state. However, we could add a txtar test and checks the /metrics endpoint, like for /health: develop/testdata/scripts/health/multi-chain.txtar

Added the test, but it does not feel that this is the right approach. The script seems too complex for txtar test. Also it's quite limited in terms of available metrics as node does not perform any actions. Automated check against soaked node might be a better option.

I was hoping it could be made simpler, but soak tests would be fine too 🤷

[cl-sonarqube-production]

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

[prashantkumar1982]

Thanks for fixing this!

[dhaidashenko]

Can we get a test demonstrating what this fixes?

We haven't had the ability to test prom metrics coherently in the past, since they are all mixed up in global state. However, we could add a txtar test and checks the /metrics endpoint, like for /health: develop/testdata/scripts/health/multi-chain.txtar

Added the test, but it does not feel that this is the right approach. The script seems too complex for txtar test. Also it's quite limited in terms of available metrics as node does not perform any actions. Automated check against soaked node might be a better option.

I was hoping it could be made simpler, but soak tests would be fine too 🤷

@jmank88 on second thought, implementation of a more generalized "framework" for metrics testing seems like a high-effort low-priority task. As the current approach gets the job done in this particular case, do you mind merging the fix as is?

[jmank88]

I was hoping it could be made simpler, but soak tests would be fine too 🤷

@jmank88 on second thought, implementation of a more generalized "framework" for metrics testing seems like a high-effort low-priority task. As the current approach gets the job done in this particular case, do you mind merging the fix as is?

Did you try using the basic stderr/stdout commands? They accept grep patterns, so it seems like this could be checked with one line for each metric, like:

curl $NODEURL/metrics

stdout 'evm_pool_rpc_node_dials_total{evmChainID="68472",nodeName="BlueEVMPrimaryNode"}'
stdout ...

I don't fully understand the extra retry logic though. Was it flakey without?

[dhaidashenko]

I was hoping it could be made simpler, but soak tests would be fine too 🤷

@jmank88 on second thought, implementation of a more generalized "framework" for metrics testing seems like a high-effort low-priority task. As the current approach gets the job done in this particular case, do you mind merging the fix as is?

Did you try using the basic stderr/stdout commands? They accept grep patterns, so it seems like this could be checked with one line for each metric, like:

curl $NODEURL/metrics

stdout 'evm_pool_rpc_node_dials_total{evmChainID="68472",nodeName="BlueEVMPrimaryNode"}'
stdout ...

I don't fully understand the extra retry logic though. Was it flakey without?

I did not observe the flake. But Dial happens in a separate goroutine, so there is no guarantee that the metric is visible, when API is ready to handle requests.