graph-chain-ethereum: Avoid adapters with errors #4468
Conversation
mangas
commented
Mar 17, 2023
•
mangas
commented
- Wire EndpointMetrics for rpc endpoints
- Use a percentage of traffic to retest errored adapters
mangas
force-pushed
the
filipe/rpc-avoid-failed-adapters
branch
3 times, most recently
from
ee5ece0
to
249f64a
Compare
mangas
force-pushed
the
filipe/rpc-avoid-failed-adapters
branch
from
249f64a
to
10d915e
Compare
| @@ -841,7 +838,7 @@ impl EthereumAdapter { | |||
| #[async_trait] | |||
| impl EthereumAdapterTrait for EthereumAdapter { | |||
| fn url_hostname(&self) -> &str { | |||
There was a problem hiding this comment.
I see only one user of this method, and it's for logs. I believe the intention is for all logs to use the provider name, so lets remove this and replace with the provider.
chain/ethereum/src/network.rs
Outdated
|
|
||
| pub fn current_error_count(&self) -> u64 { | ||
| self.endpoint_metrics | ||
| .get_count(&self.adapter.url.as_str().into()) |
There was a problem hiding this comment.
Can we use provider instead to identify the adapter, just to keep the url private?
There was a problem hiding this comment.
for adapters we can, it leaks a bit of the impl details onto config, added comments where relevant
chain/ethereum/src/network.rs
Outdated
| pub capabilities: NodeCapabilities, | ||
| adapter: Arc<EthereumAdapter>, | ||
| pub adapter: Arc<EthereumAdapter>, |
There was a problem hiding this comment.
This pub seems unecessary.
mangas
force-pushed
the
filipe/rpc-avoid-failed-adapters
branch
2 times, most recently
from
8f1ac22
to
b101e9d
Compare
There was a problem hiding this comment.
My suggestion would be that we switch to always identifying providers by name rather than url, for metrics purposes. Urls can contain sensitive information such as auth data, so it's best to restrict the places where they are exposed. So instead of ChainSection::provider_urls we'd have ChainSection::providers exposing just the provider names for the metrics.
|
Provider names won't work for firehose because we append a number to it while creating the conn_pool so providerX becomes providerX-N where N is 0..Total number of connections. Hence, you we can't dedup based on that, only the url. We could have a "parent provider" which returns providerX for all but it's quite easy to make a small change to this and just break the metrics. |
|
Having the 'parent provider' seems like a solution. You are concerned that the user changes the provider name in the config and loses the continuity of the metrics? The same could be said if the URL changes, I don't think one is more likely to change than the other. |
My concern is more about the code allowing for a dev to do the wrong thing inadvertently since the details of the conn pool leak into the different components like the metrics and deduplication |
- Wire EndpointMetrics for rpc endpoints - Use a percentage of traffic to retest errored adapters
mangas
force-pushed
the
filipe/rpc-avoid-failed-adapters
branch
3 times, most recently
from
057f16d
to
a42b4e2
Compare
mangas
force-pushed
the
filipe/rpc-avoid-failed-adapters
branch
from
a42b4e2
to
9cd8458
Compare
| &format!("{}-{}", provider.label, i), | ||
| // This label needs to be the original label so that the metrics | ||
| // can be deduped. | ||
| &provider.label, |
There was a problem hiding this comment.
This is a functional change right, instead of segregating the metrics by individual conns, they are grouped for the whole pool? Makes sense to me, but just confirming.
There was a problem hiding this comment.
The metrics will be tagged with provider/host, result, request type and so on. I changed from hosts to providers which means that the dedup needs to happen on the provider level (eg, if one conn of providerA fails, all of the providerA conns should be de-prioritised until checked again).
