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

Properly implement subgraph client fallback from local deployment to remote URL #81

Merged
merged 6 commits into from
Oct 25, 2023

Conversation

Jannis
Copy link
Collaborator

@Jannis Jannis commented Oct 23, 2023

This may deviate from the existing indexer-service, but that one has grown over time. Really, even if you index the network subgraph deployment (or escrow subgraph) locally, you just need a query URL in the indexer service. The optional deployment logic is handled by indexer-agent.

This now implements the correct logic to monitor the local subgraph deployment status and, if it's not healthy and synced, falls back to the remote URL provided. This mimics the behavior of the existing indexer service: https://github.com/graphprotocol/indexer/blob/main/packages/indexer-common/src/network-subgraph.ts#L143-L167.

@aasseman
Copy link
Contributor

Makes sense! Here's how I understood it:

  • The indexer-agent is tasked with deploying the network subgraph to the graph-node + managing fallback to a public URL.
  • The indexer-service would only need to point to the indexer-agent.

If I understood it correctly, then shouldn't that be extended further, and the SubgraphClient only point to a user-provided INDEXER_AGENT_QUERY_ENDPOINT (or similar)?

@aasseman
Copy link
Contributor

aasseman commented Oct 23, 2023

PS: and the actual path to the correct subgraph endpoint in indexer-agent would be hardcoded, ie. something like:

let indexer_agent_subgraphs_endpoint = "http://localhost:8080/subgraphs/" // (from config)
let network_subgraph = SubgraphClient::from_something(indexer_agent_subgraphs_endpoint, "escrow-subgraph");


let subgraph_url = Url::parse(subgraph_url)
.map_err(|e| anyhow!("Could not parse subgraph url `{}`: {}", subgraph_url, e))?;
pub fn new(name: &str, query_url: &str) -> Result<Self, anyhow::Error> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need name? It seems to be only used for error messages.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, no, let's not include it.

@Jannis
Copy link
Collaborator Author

Jannis commented Oct 24, 2023

Makes sense! Here's how I understood it:

  • The indexer-agent is tasked with deploying the network subgraph to the graph-node + managing fallback to a public URL.
  • The indexer-service would only need to point to the indexer-agent.

If I understood it correctly, then shouldn't that be extended further, and the SubgraphClient only point to a user-provided INDEXER_AGENT_QUERY_ENDPOINT (or similar)?

I re-checked what the existing indexer-service does and it's a bit more complicated. It essentially polls the status of the local deployment and uses that if synced, otherwise it uses the remote subgraph. I think we need at least a local and remote URL for the network subgraph (same with escrow ideally). I'll rework this PR to match the existing behavior.

@github-actions
Copy link
Contributor

Pull Request Test Coverage Report for Build 6628418855

  • 119 of 239 (49.79%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.7%) to 41.309%

Changes Missing Coverage Covered Lines Changed/Added Lines %
common/src/subgraph_client/client.rs 98 124 79.03%
service/src/main.rs 0 28 0.0%
common/src/subgraph_client/monitor.rs 0 66 0.0%
Totals Coverage Status
Change from base Build 6590531530: -1.7%
Covered Lines: 1010
Relevant Lines: 2445

💛 - Coveralls

@Jannis Jannis changed the title refactor: only support a query URL in SubgraphClient Properly implement subgraph client fallback from local deployment to remote URL Oct 24, 2023
This is how the existing indexer-common does it - it uses the local
network subgraph if it's synced and healthy, otherwise falls back to a
remote subgraph URL.
@Jannis Jannis force-pushed the jannis/simplify-subgraph-client branch from ac6dcf5 to 460c6bd Compare October 24, 2023 14:50
@aasseman
Copy link
Contributor

Makes sense! Here's how I understood it:

  • The indexer-agent is tasked with deploying the network subgraph to the graph-node + managing fallback to a public URL.
  • The indexer-service would only need to point to the indexer-agent.

If I understood it correctly, then shouldn't that be extended further, and the SubgraphClient only point to a user-provided INDEXER_AGENT_QUERY_ENDPOINT (or similar)?

I re-checked what the existing indexer-service does and it's a bit more complicated. It essentially polls the status of the local deployment and uses that if synced, otherwise it uses the remote subgraph. I think we need at least a local and remote URL for the network subgraph (same with escrow ideally). I'll rework this PR to match the existing behavior.

Oh, ok then!

aasseman
aasseman previously approved these changes Oct 24, 2023
Copy link
Contributor

@aasseman aasseman left a comment

Choose a reason for hiding this comment

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

LGTM. Just one comment in the code that may be obsolete.

common/src/subgraph_client/client.rs Outdated Show resolved Hide resolved
@Jannis
Copy link
Collaborator Author

Jannis commented Oct 25, 2023

Added a bunch more tests for the fallback behavior. This should be good to go now.

Copy link
Contributor

@aasseman aasseman left a comment

Choose a reason for hiding this comment

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

:shipit:

@Jannis Jannis merged commit 407c17d into main Oct 25, 2023
4 checks passed
@Jannis Jannis deleted the jannis/simplify-subgraph-client branch October 25, 2023 18:50
@aasseman aasseman mentioned this pull request Oct 31, 2023
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.

2 participants