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

feat(gateway): IPFSBackend metrics and HTTP range support #245

Merged
merged 4 commits into from
Apr 3, 2023

Conversation

lidel
Copy link
Member

@lidel lidel commented Mar 31, 2023

This PR is part of ipfs-inactive/bifrost-gateway#61

  • Moves metric-related code to gateway/metrics.go
  • Adds opentelemetry span for every IPFSBackend API call
  • Adds success/failure histogram for each API call ipfs_gw_backend_api_call_duration_seconds

TODO

- Adds spans for every IPFSBackend API call
- Adds success/failure histogram for each API call
  ipfs_gw_backend_api_call_duration_seconds
@codecov
Copy link

codecov bot commented Mar 31, 2023

Codecov Report

Merging #245 (de9daf5) into main (92407c6) will increase coverage by 0.23%.
The diff coverage is 64.38%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
gateway/gateway.go 88.52% <ø> (ø)
gateway/metrics.go 62.50% <62.50%> (ø)
gateway/handler_defaults.go 36.66% <70.96%> (+5.15%) ⬆️
blockservice/blockservice.go 73.56% <100.00%> (ø)
gateway/blocks_gateway.go 46.64% <100.00%> (+2.09%) ⬆️
gateway/handler.go 56.04% <100.00%> (-6.99%) ⬇️
gateway/handler_unixfs_dir.go 62.89% <100.00%> (ø)

... and 11 files with indirect coverage changes

lidel added a commit to ipfs-inactive/bifrost-gateway that referenced this pull request Mar 31, 2023
- 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.
lidel added a commit to ipfs/kubo that referenced this pull request Mar 31, 2023
lidel added a commit to ipfs/kubo that referenced this pull request Mar 31, 2023
@lidel lidel changed the title feat(gateway): IPFSBackend tracing and metrics feat(gateway): IPFSBackend metrics and HTTP range support Mar 31, 2023
lidel added a commit to ipfs/kubo that referenced this pull request Mar 31, 2023
Exposing range fixes and new metrics from
ipfs/boxo#245
lidel added a commit to ipfs/kubo that referenced this pull request Mar 31, 2023
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
lidel added a commit to ipfs/kubo that referenced this pull request Apr 1, 2023
Exposing range fixes and new metrics from
ipfs/boxo#245
Comment on lines +136 to +140
md, rc, errCh, err := b.api.GetCAR(ctx, path)

// TODO: handle errCh
b.updateApiCallMetric(name, err, begin)
return md, rc, errCh, err
Copy link
Member Author

@lidel lidel Apr 1, 2023

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)

Copy link
Member Author

@lidel lidel left a 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")
Copy link
Member Author

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 ;)

Copy link
Member Author

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)
Copy link
Member Author

@lidel lidel Apr 1, 2023

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

@lidel lidel marked this pull request as ready for review April 1, 2023 01:04
@lidel lidel requested a review from a team as a code owner April 1, 2023 01:04
Copy link
Member

@hacdias hacdias left a 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!

@hacdias hacdias merged commit 5a2c987 into main Apr 3, 2023
@hacdias hacdias deleted the feat/backend-api-metrics branch April 3, 2023 07:46
lidel added a commit that referenced this pull request Apr 5, 2023
removed depecated and unused TTFB histograms.
after #176
and #245
we now have api_call_duration_seconds histograms for higher resolution info
per backend operation
lidel added a commit that referenced this pull request Apr 5, 2023
removed depecated and unused TTFB histograms.
after #176
and #245
we now have api_call_duration_seconds histograms for higher resolution info
per backend operation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants