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

Local Graph node debug tool using a remote GraphQL endpoint (per LimeChain) #2995

Merged
merged 45 commits into from
Feb 10, 2022
Merged

Conversation

VIVelev
Copy link
Contributor

@VIVelev VIVelev commented Nov 23, 2021

This PR implements the Debug Tool discussed with LimeChain.

What is it?

A way to debug your failed subgraph at block X without needing to wait to sync up to block X.

How it is done?

It uses a remote Graph node GraphQL endpoint that is guaranteed to have the subgraph indexed up to block X in order to provide the locally-deployed subgraph being debugged a synced-up store.

What changes have been made?

  • store_get in runtime/wasm/src/module/mod.rs has been changed to resort to the endpoint when needed.
  • There is a new file: runtime/wasm/src/module/debug_tool.rs, that's were the main logic is.
  • The subgraphs.subgraph_manifest PostgreSQL table has been altered to include a debug_endpoint string, nullable column to store the endpoint's address.
  • The SubgraphStore trait and related have been modified to enable easy retrieval of debug_endpoint when needed. (most prominently a new method get_debug_endpoint has been added)
  • New JSON-RPC handler (debug_handler) to communicate with graph-cli's graph debug
  • SubgraphRegistrar's create_subgraph_version has been altered to accommodate the 2 new params: debug_endpoint and debug_block_number.

Points to discuss

  • Should create_subgraph_version be split into 2 separate functions - one for deploy only and one for debug only?

@evaporei evaporei self-requested a review November 30, 2021 11:29
@evaporei
Copy link
Contributor

I see there are a lot of changes in the imports (ordering, position, etc), is this being by your IDE somehow?

@VIVelev
Copy link
Contributor Author

VIVelev commented Nov 30, 2021

Actually those changes were not made by me, so I am not sure. They are coming from 9b0cdcf. Judging by the commit message, I suppose they are caused by Rustfmt?

cc: @axiomatic-aardvark

@evaporei
Copy link
Contributor

evaporei commented Nov 30, 2021

@VIVelev I don't think it's rustfmt because we have it running on CI, if the master branch has incorrect formatting, the CI wouldn't pass.

Or if is rustfmt, it might be one with different configuration (graph-node uses the default one).

Copy link
Contributor

@evaporei evaporei left a comment

Choose a reason for hiding this comment

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

Nice PR! I commented on suggestions and required changes 🙂

runtime/wasm/Cargo.toml Outdated Show resolved Hide resolved
runtime/wasm/src/asc_abi/class.rs Outdated Show resolved Hide resolved
core/src/subgraph/registrar.rs Outdated Show resolved Hide resolved
graph/src/components/store.rs Outdated Show resolved Hide resolved
runtime/wasm/src/module/debug_tool.rs Outdated Show resolved Hide resolved
runtime/wasm/src/module/debug_tool.rs Outdated Show resolved Hide resolved
runtime/wasm/src/module/mod.rs Outdated Show resolved Hide resolved
runtime/wasm/src/module/debug_tool.rs Outdated Show resolved Hide resolved
runtime/wasm/src/module/debug_tool.rs Outdated Show resolved Hide resolved
@VIVelev VIVelev requested a review from evaporei December 1, 2021 12:03
Copy link
Contributor

@evaporei evaporei left a comment

Choose a reason for hiding this comment

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

Hey @VIVelev thanks for fixing the issues and restructuring your PR ❤️
I'm really sorry it took so long to look over it again.

The only two points I'm not 100% sure about are:

  1. If the Mutex is the best way to handle the already saved/fetched ids
  2. I see that you just pass a DeploymentHash through the graph-cli (graph deploy, CLI PR for reference) and no block number like grafting. Even though it makes sense in the scenario of a failing subgraph because the block number will be the last one (the one that failed), if someone wants to debug-fork for some reason in a different point in time, this PR doesn't allow that. I think you could add this for better flexibility of the feature.

But before changing any of the two points above, I would like to know @Jannis opinion 😊

store/postgres/src/deployment.rs Outdated Show resolved Hide resolved
runtime/wasm/src/module/mod.rs Show resolved Hide resolved
client: reqwest::Client,
endpoint: Url,
schema: Arc<Schema>,
fetched_ids: Mutex<HashSet<String>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

The implementation of the fetching mechanism seems correct, @Jannis do you see any problem with using a Mutex here? 🤔

@@ -170,6 +170,32 @@ impl TryFrom<q::Value> for Value {
}
}

impl From<serde_json::Value> for Value {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused why there's the need for two Values in the fork.rs file, is this really needed or could we just do one JSON transformation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The field types supported by serde_json::Value and the Value used in the subgraph store are different, so there needs to be a mechanism in which to convert from one to the other. The above Value and the From<serde_json::Value> provide this mechanism.

store/postgres/src/fork.rs Outdated Show resolved Hide resolved
@VIVelev
Copy link
Contributor Author

VIVelev commented Dec 15, 2021

Regarding 2., I decided to leave the block number out of the CLI, since the user can pass it through dataSources.source.startBlock in the subgraph manifest. It seems like adding startBlock to the CLI duplicates functionality and makes things confusing imo.

@leoyvens
Copy link
Collaborator

leoyvens commented Jan 4, 2022

#2414 likely generated conflicts for this, lmk if you have issues when rebasing.

@VIVelev
Copy link
Contributor Author

VIVelev commented Jan 5, 2022

#2414 likely generated conflicts for this, lmk if you have issues when rebasing.

It's all good now :)

store/postgres/src/fork.rs Outdated Show resolved Hide resolved
@VIVelev VIVelev requested review from leoyvens and evaporei January 11, 2022 03:23
@leoyvens leoyvens dismissed their stale review January 11, 2022 16:23

minor review, addressed

@evaporei evaporei merged commit f4e6992 into graphprotocol:master Feb 10, 2022
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.

4 participants