Add IPFS usage metrics / extend logging / extend supported content path formats by isum · Pull Request #6058 · graphprotocol/graph-node

isum · 2025-06-12T10:27:10Z

This PR introduces the following improvements to IPFS:

Adds 4 new metrics grouped by deployment hashes:
- ipfs_request_count - shows the total number of IPFS requests
- ipfs_error_count - shows the total number of failed IPFS requests
- ipfs_not_found_count - shows the total number of IPFS requests that timed out
- ipfs_request_duration - shows the duration of successful IPFS requests
Fixes a minor bug that could cause the IPFS gateway client to fail during initialization
The IPFS content path parser now supports the following formats:
- <CID>[/<path>]
- /ipfs/<CID>[/<path>]
- ipfs://<CID>[/<path>]
- http[s]://.../ipfs/<CID>[/<path>]
- http[s]://.../api/v0/cat?arg=<CID>[/<path>]
- http[s]://.../<CID>[/<path>]
IPFS error logs now include more context, such as deployment hash and the requested IPFS path

lutter

Nice improvements! It would be good to address the comments about the Default implementation and ownership of IpfsContext before merging, but feel free to merge once that's done.

lutter · 2025-06-19T21:33:24Z

chain/substreams/src/data_source.rs

        let logger = Logger::root(Discard, o!());
-        let ds: DataSource = ds.resolve(&link_resolver, &logger, 0).await.unwrap();
+        let ds: DataSource = ds
+            .resolve(&Default::default(), &link_resolver, &logger, 0)


I kinda dislike using Default::default because from just reading the code, I have no idea what gets passed there. Could you write that as LinkResolverContext::default() ?

Updated to DeploymentHash::default()

lutter · 2025-06-19T21:33:37Z

chain/substreams/src/data_source.rs

        let logger = Logger::root(Discard, o!());
-        let ds: DataSource = ds.resolve(&link_resolver, &logger, 0).await.unwrap();
+        let ds: DataSource = ds
+            .resolve(&Default::default(), &link_resolver, &logger, 0)


Same comment about Default::default() here

Updated to DeploymentHash::default()

lutter · 2025-06-19T21:46:06Z

graph/src/ipfs/content_path.rs

+        assert_eq!(path, make_path(CID_V0, None));
+
+        let path = ContentPath::new(format!("https://ipfs.com/{CID_V0}/readme.md")).unwrap();
+        assert_eq!(path, make_path(CID_V0, Some("readme.md")));


Everything in this file looks great, but since we are handling user input here, are there ways to abuse this in security-relevant ways? (I think the answer is 'no', but wanted to double-check)

We are not making HTTP requests to arbitrary URLs here, and CIDs are parsed and must be valid. The only part that remains unchanged is the optional path after the CID, but it does not seem to be possible to abuse that it a security-relevant way

lutter · 2025-06-19T21:49:35Z

graph/src/components/link_resolver/mod.rs

+            deployment_hash: "test".into(),
+            logger: crate::log::discard(),
+        }
+    }


This seems petty, but rather than implement Default, it might be clearer to make this a method LinkResolverContext::dummy() (only in debug builds) It's not really a default implementation, since there's not really a sensible default for this struct.

Added a dedicated test() method to create a test LinkResolverContext

lutter · 2025-06-19T22:12:54Z

core/src/polling_monitor/ipfs_service.rs

-pub type IpfsService = Buffer<ContentPath, BoxFuture<'static, Result<Option<Bytes>, Error>>>;
+pub type IpfsService = Buffer<IpfsRequest, BoxFuture<'static, Result<Option<Bytes>, Error>>>;
+
+#[derive(Clone, Debug)]


I got a little hung up on why this needs to be 'Clone'; I think it ultimately boils down to the fact that when we make the request, we want it returned back and we do that in ReturnRequest::call; it seems we could avoid cloning if we changed what IpfsServiceInner::call_inner returns and have it always include the request in its return value, basically moving ownership of the request through call_inner. In this case, I am also not sure how important it is to save on cloning, though we clone on every request.

In any event, this would definitely be something for a different PR.

I made the request CheapClone by using Arc internally in ContentPath, and that will help a bit. Optimizing the polling monitor is a good idea and I will look into this as part of different issue / PR.

lutter · 2025-06-19T22:13:43Z

graph/src/ipfs/client.rs

    /// The timeout is not propagated to the resulting stream.
    async fn cat_stream(
        self: Arc<Self>,
+        ctx: IpfsContext,


Does this need to take ownership? It seems that just a reference would be enough

Updated this to accept &IpfsContext

lutter · 2025-06-19T22:13:58Z

graph/src/ipfs/client.rs

    /// does not return a response within the specified amount of time.
    async fn cat(
        self: Arc<Self>,
+        ctx: IpfsContext,


Same question about ownership

Updated this to accept &IpfsContext

lutter · 2025-06-19T22:14:12Z

graph/src/ipfs/client.rs

    /// does not return a response within the specified amount of time.
    async fn get_block(
        self: Arc<Self>,
+        ctx: IpfsContext,


Same question about ownership

Updated this to accept &IpfsContext

lutter · 2025-08-20T16:20:00Z

Do we still want these metrics? If not, can we close this PR?

…tests

isum self-assigned this Jun 12, 2025

isum added the area/ipfs label Jun 12, 2025

isum mentioned this pull request Jun 12, 2025

graph: Log the path we are accessing in IPFS errors #6032

Closed

fordN requested a review from lutter June 12, 2025 17:24

lutter approved these changes Jun 19, 2025

View reviewed changes

isum added 8 commits September 18, 2025 21:46

add ipfs metrics

e1966e0

add ipfs context and use metrics

91370e0

extend ipfs content path to support multiple input formats

17b37e6

fix release build

dddedff

fix ipfs gateway tests

9fb5b2b

fix(ipfs): make IPFS accept a reference to the context

e7ad4bd

fix(ipfs): create test context and metrics using dedicated methods

5746ccb

fix(ipfs): use type name when creating a default deployment hash for …

5342293

…tests

isum force-pushed the ion/add-ipfs-logs-and-metrics branch from ea20209 to 5342293 Compare September 19, 2025 10:18

isum added 2 commits September 19, 2025 13:34

fix(ipfs): update tests after rebase

1053282

fix(tests): fix release build

3f74611

isum force-pushed the ion/add-ipfs-logs-and-metrics branch from 2eccf5c to 3f74611 Compare September 19, 2025 10:52

isum merged commit 4915b6f into master Sep 19, 2025
6 checks passed

isum deleted the ion/add-ipfs-logs-and-metrics branch September 19, 2025 10:59

Comments

Conversation

isum commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lutter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

isum Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lutter commented Aug 20, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

isum commented Jun 12, 2025 •

edited

Loading

isum Sep 18, 2025 •

edited

Loading