Skip to content
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

Improve rpc errors logging #6362

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

friofry
Copy link
Contributor

@friofry friofry commented Feb 25, 2025

While debugging status-im/status-mobile#22155 I needed more information in the logs.

So this PR addresses 2 issues:

  • No last error in geth.logs (needed for debugging RPC provider issues)
  • No reliable way to map ProviderStatus to an RpcProvider from networks (needed by @dlipicar)

Changes:

  • Add rpc method name to error message (logs and health manager status)
  • Add chainID to ProviderStatus
  • Fix health-manager last error messages in geth.log
  • Removed unused types
  • Use ProviderName as key in health-manager (since circuit name doesn't map directly to a provider and could be empty in some cases)

Closes #6266

@status-im-auto
Copy link
Member

status-im-auto commented Feb 25, 2025

Jenkins Builds

Click to see older builds (8)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 071941f #1 2025-02-25 10:38:44 ~3 min android 📦aar
✖️ 071941f #1 2025-02-25 10:39:54 ~4 min tests 📄log
✔️ 071941f #1 2025-02-25 10:40:19 ~4 min windows 📦zip
✔️ 071941f #1 2025-02-25 10:40:40 ~5 min linux 📦zip
✔️ 071941f #1 2025-02-25 10:41:32 ~6 min macos 📦zip
✔️ 071941f #1 2025-02-25 10:41:49 ~6 min macos 📦zip
✔️ 071941f #1 2025-02-25 10:43:02 ~7 min ios 📦zip
✔️ 071941f #1 2025-02-25 10:48:35 ~13 min tests-rpc 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✖️ c79dddc #2 2025-02-25 10:43:34 ~2 min tests 📄log
✔️ c79dddc #2 2025-02-25 10:44:02 ~3 min android 📦aar
✔️ c79dddc #2 2025-02-25 10:44:10 ~3 min windows 📦zip
✔️ c79dddc #2 2025-02-25 10:45:52 ~4 min macos 📦zip
✔️ c79dddc #2 2025-02-25 10:46:05 ~5 min linux 📦zip
✔️ c79dddc #2 2025-02-25 10:46:50 ~3 min ios 📦zip
✔️ c79dddc #2 2025-02-25 10:48:13 ~6 min macos 📦zip
✔️ c79dddc #2 2025-02-25 11:01:27 ~12 min tests-rpc 📄log
✔️ f24af90 #3 2025-02-26 12:47:02 ~2 min ios 📦zip
✔️ f24af90 #3 2025-02-26 12:47:19 ~3 min android 📦aar
✔️ f24af90 #3 2025-02-26 12:47:30 ~3 min windows 📦zip
✔️ f24af90 #3 2025-02-26 12:48:07 ~3 min macos 📦zip
✖️ f24af90 #3 2025-02-26 12:48:21 ~4 min tests 📄log
✔️ f24af90 #3 2025-02-26 12:50:09 ~5 min macos 📦zip
✔️ f24af90 #3 2025-02-26 12:50:18 ~6 min linux 📦zip
✔️ f24af90 #3 2025-02-26 12:57:13 ~12 min tests-rpc 📄log

}

// NewProviderStatus processes ProviderCallStatus and returns a new ProviderStatus.
func NewProviderStatus(res ProviderCallStatus) ProviderStatus {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method was only used in tests.

)

// CreateEthClientFromProvider creates an Ethereum RPC client from the given RpcProvider.
func CreateEthClientFromProvider(provider params.RpcProvider, rpcUserAgentName string) (*rpc.Client, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is extracted from rpc/client.go to have a clear way of creating rpc.Client from RpcProvider.

@@ -15,32 +17,77 @@ import (
type RPSLimitedEthClientInterface interface {
EthClientInterface
GetLimiter() *rpclimiter.RPCRpsLimiter
GetName() string
CopyWithName(name string) RPSLimitedEthClientInterface
GetCircuitName() string
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name was always a circuit name, so renamed to avoid ambiguity.
Multiple providers can share the same circuit, hence 2 properties Circuit and Provider.

return NewRPSLimitedEthClient(c.rpcClient, c.limiter, circuitName, c.providerName)
}

func (c *RPSLimitedEthClient) ExecuteWithRPSLimit(f func(client EthClientInterface) (interface{}, error)) (interface{}, error) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted this function from rpc/chain/client.go as this logic is more related to RpsLimitedEthClient than chain client.

@@ -44,6 +44,7 @@ func (a *Aggregator) Update(providerStatus rpcstatus.ProviderStatus) {
// Update existing provider status or add a new provider.
if ps, exists := a.providerStatuses[providerStatus.Name]; exists {
ps.Status = providerStatus.Status
ps.ChainID = providerStatus.ChainID
Copy link
Contributor

@dlipicar dlipicar Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this layer supposed to be chainID-agnostic? (as in, we don't deal with chainIDs with market providers, but we do once we go into a more specific layer of RPC/collectibles providers)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh you just removed the chainID? If it is useful for logging purposes we can surely add it in some more specialized form of Aggregator

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, I realised that it's OK to remove chainID from the provider state because we have chainID as a key in the states map

@friofry friofry requested a review from dlipicar February 26, 2025 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add RPCProvider unique ID to HealthChecker chain statuses
3 participants