feat: add server timing to responses #164
Conversation
|
with ipfs/service-worker-gateway#531, requesting http://non-ipns-site.blog.ipns.helia-sw-gateway.localhost/ results in: 
|
There was a problem hiding this comment.
This is pretty neat. Implementation looks good to me.
What do you think about making this optional per request? I can imagine that in node.js this isn't very useful and just makes the response object larger.
We need to work out exactly what events we need to time
At the very least we'd want:
- IPNS/DNSLink resolution (if it's an ipns url)
- content routing, i.e. finding providers.
We could look into making it an option and defaulting to true in sw-gateway and helia-http-gateway. FYI with helia-http-gateway, which runs on nodejs, it's still useful because it's returning the response directly from verified-fetch.
more granular IPNS/DNSLink resolution will require some deeper piping of the server timing. I will look into those, or see if I can finagle some determinism from onProgress events |
|
@2color added the option to include server timing headers, defaulting to false on construction but overridable per request. Also added docs and updated readme |
| @@ -452,6 +460,26 @@ export class VerifiedFetch { | |||
| [identity.code]: this.handleRaw | |||
| } | |||
|
|
|||
| private async handleServerTiming<T> (name: string, description: string, fn: () => Promise<T>): Promise<T> { | |||
There was a problem hiding this comment.
Can we check this.includeServerTiming here to encapsulate most of the logic here and avoid the measuring and assigning of this.serverTimingHeaders?
That would also allow simplifying responseWithServerTiming and renaming to something a bit more generic, e.g. augmentResponse or handleResponse
There was a problem hiding this comment.
yea let me peek at this
There was a problem hiding this comment.
so, it just so happened that switching the server timing handling to handleServerTiming required me to pipe things through a lot more, so I'm moving forward with this and adding the "IPNS/DNSLink resolution (if it's an ipns url)" you previously mentioned.
I didn't originally like it in handleServerTiming because it required plumbing the request-level "withServerTiming" into more places, but it makes more sense for multiple reasons.
it also prevents the extra work of wrapping the timed functions.. so it's a win. thanks for the push
Co-authored-by: Daniel Norman <[email protected]>
|
@2color all requested changes are in, but the code is a little bit sloppier now because of how parse-resource & parse-url-string are currently written. |
There was a problem hiding this comment.
LGTM. I still think it would be really useful to add a duration for finding providers from each router/block-broker.
Since that would probably require us to add progress hooks to the helia routers, we can handle this in a follow up PR.
Not directly related, but more insight into routing would help surface more nuanced errors: no providers vs. failure to connect to providers, to the user.
|
CI seems to be failing when attempting to install Kubo on windows.. i'm looking into it and will try clearing package-lock and updating Kubo |
Nikolai is looking into the collab cluster |
## [@helia/verified-fetch-v2.4.0](https://github.com/ipfs/helia-verified-fetch/compare/@helia/verified-fetch-2.3.1...@helia/verified-fetch-2.4.0) (2025-01-14) ### Features * add server timing to responses ([#164](#164)) ([0701c71](0701c71))
|
🎉 This PR is included in version @helia/verified-fetch-v2.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Title
feat: add server timing to responses
Description
This PR adds Server Timing headers to responses by wrapping all responses returned by
fetchin aResponseobject that includes the server timing headers.The headers are generated by wrapping async functions with a
serverTimingutility function that records the time taken to execute the function, and then adding those headers to an array on the VerifiedFetch class.Notes & open questions
serverTimingutility function usage so that it's easier to add new timings without having to handle the error and success cases manually for each one.Example of current server timings for http://bafybeie4vcqkutumw7s26ob2bwqwqi44m6lrssjmiirlhrzhs2akdqmkw4.ipfs.helia-sw-gateway.localhost/:
Example of current server timings for http://blog-ipfs-tech.ipns.helia-sw-gateway.localhost/:
Change checklist