Add IPFS usage metrics / extend logging / extend supported content path formats #6058
Conversation
| @@ -433,7 +444,10 @@ mod test { | |||
| let ds: UnresolvedDataSource = serde_yaml::from_str(TEMPLATE_DATA_SOURCE).unwrap(); | |||
| let link_resolver: Arc<dyn LinkResolver> = Arc::new(NoopLinkResolver {}); | |||
| 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) | |||
There was a problem hiding this comment.
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() ?
| @@ -470,7 +484,10 @@ mod test { | |||
| serde_yaml::from_str(TEMPLATE_DATA_SOURCE_WITH_PARAMS).unwrap(); | |||
| let link_resolver: Arc<dyn LinkResolver> = Arc::new(NoopLinkResolver {}); | |||
| 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) | |||
There was a problem hiding this comment.
Same comment about Default::default() here
| 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"))); |
There was a problem hiding this comment.
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)
| deployment_hash: "test".into(), | ||
| logger: crate::log::discard(), | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| 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)] |
There was a problem hiding this comment.
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.
| @@ -32,21 +32,31 @@ pub trait IpfsClient: Send + Sync + 'static { | |||
| /// The timeout is not propagated to the resulting stream. | |||
| async fn cat_stream( | |||
| self: Arc<Self>, | |||
| ctx: IpfsContext, | |||
There was a problem hiding this comment.
Does this need to take ownership? It seems that just a reference would be enough
| @@ -61,27 +71,33 @@ pub trait IpfsClient: Send + Sync + 'static { | |||
| /// does not return a response within the specified amount of time. | |||
| async fn cat( | |||
| self: Arc<Self>, | |||
| ctx: IpfsContext, | |||
There was a problem hiding this comment.
Same question about ownership
| @@ -95,26 +111,32 @@ pub trait IpfsClient: Send + Sync + 'static { | |||
| /// does not return a response within the specified amount of time. | |||
| async fn get_block( | |||
| self: Arc<Self>, | |||
| ctx: IpfsContext, | |||
There was a problem hiding this comment.
Same question about ownership
This PR introduces the following improvements to IPFS:
ipfs_request_count- shows the total number of IPFS requestsipfs_error_count- shows the total number of failed IPFS requestsipfs_not_found_count- shows the total number of IPFS requests that timed outipfs_request_duration- shows the duration of successful IPFS requests<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>]Closes #6036