-
Notifications
You must be signed in to change notification settings - Fork 86
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
feat(gateway): IPFSBackend metrics and HTTP range support #245
Conversation
- Adds spans for every IPFSBackend API call - Adds success/failure histogram for each API call ipfs_gw_backend_api_call_duration_seconds
Codecov Report
@@ Coverage Diff @@
## main #245 +/- ##
==========================================
+ Coverage 47.88% 48.12% +0.23%
==========================================
Files 269 271 +2
Lines 32976 33159 +183
==========================================
+ Hits 15792 15957 +165
- Misses 15513 15534 +21
+ Partials 1671 1668 -3
|
- IPFSBackend metrics from ipfs/boxo#245 - GraphGateway metrics from #61 - Version for tracking rollouts
Also fixes serving range requests for /ipfs/bafydir when we know we'll get the index.html or _redirect.
Exposing metrics from ipfs/boxo#245
Exposing metrics from ipfs/boxo#245
Exposing range fixes and new metrics from ipfs/boxo#245
Exposing range fixes and new metrics from ipfs/boxo#245
blockstore can be used in contexts where there is no bitswap, and this log message will me EXTREMELY confusing, wasting people's time on debugging invalid side of system
Exposing range fixes and new metrics from ipfs/boxo#245
md, rc, errCh, err := b.api.GetCAR(ctx, path) | ||
|
||
// TODO: handle errCh | ||
b.updateApiCallMetric(name, err, begin) | ||
return md, rc, errCh, err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 (late friday wrap-up) @aschmahmann updateApiCallMetric
will increase counter for success or failure based on err
. Are we missing anything critical if we only check err
here and ignore errCh
?
(This should not block this PR, we can fix metric later)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aschmahmann @hacdias I think this is ready for review.
TLDR: adds metrics, fixes range requests, merges Get
and GetRange
into one func.
Sharness in Kubo PR is green: ipfs/kubo#9786
@@ -244,7 +244,7 @@ func getBlock(ctx context.Context, c cid.Cid, bs blockstore.Blockstore, fget fun | |||
|
|||
// TODO be careful checking ErrNotFound. If the underlying | |||
// implementation changes, this will break. | |||
logger.Debug("Blockservice: Searching bitswap") | |||
logger.Debug("BlockService: Searching") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ there is no bitswap in bifrost-gateway, yet we got super confusing "Searching bitswap" message here ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ moved metrics code here
// - A range request for a directory currently holds no semantic meaning. | ||
// | ||
// [HTTP Byte Ranges]: https://httpwg.org/specs/rfc9110.html#rfc.section.14.1.2 | ||
Get(context.Context, ImmutablePath, ...ByteRange) (ContentPathMetadata, *GetResponse, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ removed GetRange
, we now have Get
with optional range requests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this. I tested it locally and it indeed solves the range requests. It LGTM. Nice call to move the metrics stuff to a separate file. It was quite a clutter!
This PR is part of ipfs-inactive/bifrost-gateway#61
gateway/metrics.go
TODO
Get
andGetRange
are now the same, as suggested in feat: switch gateway GetRange API to match Get #240 (review)