refactor: merge Get and GetRange

ipfs/boxo#240 (review)

blockstore_proxy.go

@@ -12,7 +12,7 @@ import (
 
 	"github.com/ipfs/bifrost-gateway/lib"
 	blockstore "github.com/ipfs/boxo/blockstore"
-	"github.com/ipfs/go-block-format"
+	blocks "github.com/ipfs/go-block-format"
 	"github.com/ipfs/go-cid"
 )
 

go.mod

@@ -7,7 +7,7 @@ require (
 	github.com/filecoin-saturn/caboose v0.0.0-20230329203940-7c08345c244d
 	github.com/gogo/protobuf v1.3.2
 	github.com/hashicorp/golang-lru/v2 v2.0.1
-	github.com/ipfs/boxo v0.8.0-rc3.0.20230331021433-e49cc43d95ad
+	github.com/ipfs/boxo v0.8.0-rc3.0.20230401000028-de9daf5fc542
 	github.com/ipfs/go-block-format v0.1.2
 	github.com/ipfs/go-cid v0.4.0
 	github.com/ipfs/go-ipld-format v0.4.0

go.sum

@@ -186,8 +186,8 @@ github.com/inconshreveable/mousetrap v1.0.1 h1:U3uMjPSQEBMNp1lFxmllqCPM6P5u/Xq7P
 github.com/inconshreveable/mousetrap v1.0.1/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw=
 github.com/ipfs/bbloom v0.0.4 h1:Gi+8EGJ2y5qiD5FbsbpX/TMNcJw8gSqr7eyjHa4Fhvs=
 github.com/ipfs/bbloom v0.0.4/go.mod h1:cS9YprKXpoZ9lT0n/Mw/a6/aFV6DTjTLYHeA+gyqMG0=
-github.com/ipfs/boxo v0.8.0-rc3.0.20230331021433-e49cc43d95ad h1:olq3866OLC+aCW3hImmUtze0Nh8lbFfciTSeq09xfQU=
-github.com/ipfs/boxo v0.8.0-rc3.0.20230331021433-e49cc43d95ad/go.mod h1:RIsi4CnTyQ7AUsNn5gXljJYZlQrHBMnJp94p73liFiA=
+github.com/ipfs/boxo v0.8.0-rc3.0.20230401000028-de9daf5fc542 h1:2aYk8I/b1NvD2AzsNMhFrCbPjzuarcHVBdGtl0KRST4=
+github.com/ipfs/boxo v0.8.0-rc3.0.20230401000028-de9daf5fc542/go.mod h1:RIsi4CnTyQ7AUsNn5gXljJYZlQrHBMnJp94p73liFiA=
 github.com/ipfs/go-bitfield v1.1.0 h1:fh7FIo8bSwaJEh6DdTWbCeZ1eqOaOkKFI74SCnsWbGA=
 github.com/ipfs/go-bitfield v1.1.0/go.mod h1:paqf1wjq/D2BBmzfTVFlJQ9IlFOZpg422HL0HqsGWHU=
 github.com/ipfs/go-block-format v0.0.2/go.mod h1:AWR46JfpcObNfg3ok2JHDUfdiHRgWhJgCQF+KIgOPJY=

lib/blockstore_cache.go

@@ -19,7 +19,7 @@ import (
 
 const DefaultCacheBlockStoreSize = 1024
 
-var cacheLog = golog.Logger("cache-blockstore")
+var cacheLog = golog.Logger("cache/block")
 
 func NewCacheBlockStore(size int) (blockstore.Blockstore, error) {
 	c, err := lru.New2Q[string, []byte](size)

lib/graph_gateway.go

@@ -36,7 +36,7 @@ import (
 	"go.uber.org/multierr"
 )
 
-var graphLog = golog.Logger("graph-gateway")
+var graphLog = golog.Logger("backend/graph")
 
 // type DataCallback = func(resource string, reader io.Reader) error
 // TODO: Don't use a caboose type, perhaps ask them to use a type alias instead of a type
@@ -201,7 +201,7 @@ func registerGraphGatewayMetrics() *GraphGatewayMetrics {
 		Subsystem: "gw_graph_backend",
 		Name:      "car_fetch_params",
 		Help:      "How many times specific CAR parameter was used during CAR data fetch.",
-	}, []string{"depth", "bytes"})
+	}, []string{"depth", "ranges"}) // we use 'ranges' instead of 'bytes' here because we only caount the number of ranges present
 	prometheus.MustRegister(carParamsMetric)
 
 	return &GraphGatewayMetrics{
@@ -241,7 +241,7 @@ func (api *GraphGateway) loadRequestIntoSharedBlockstoreAndBlocksGateway(ctx con
 		defer func() {
 			if r := recover(); r != nil {
 				// TODO: move to Debugw?
-				graphLog.Errorw("Recovered fetcher error", "error", r)
+				graphLog.Errorw("Recovered fetcher error", "path", path, "error", r)
 			}
 		}()
 		metrics.carFetchAttemptMetric.Inc()
@@ -266,11 +266,9 @@ func (api *GraphGateway) loadRequestIntoSharedBlockstoreAndBlocksGateway(ctx con
 			}
 		})
 		if err != nil {
-			// TODO: move to debug?
-			graphLog.Errorw("car Fetch failed", "error", err)
+			graphLog.Debugw("car Fetch failed", "path", path, "error", err)
 		}
 		if err := carFetchingExch.Close(); err != nil {
-			// TODO: move to debug?
 			graphLog.Errorw("carFetchingExch.Close()", "error", err)
 		}
 		doneWithFetcher <- struct{}{}
@@ -340,36 +338,17 @@ func wrapNodeWithClose[T files.Node](node T, closeFn func()) (T, error) {
 	}
 }
 
-func (api *GraphGateway) Get(ctx context.Context, path gateway.ImmutablePath) (gateway.ContentPathMetadata, *gateway.GetResponse, error) {
-	api.metrics.carParamsMetric.With(prometheus.Labels{"depth": "1", "bytes": ""}).Inc()
-	blkgw, closeFn, err := api.loadRequestIntoSharedBlockstoreAndBlocksGateway(ctx, path.String()+"?format=car&depth=1")
-	if err != nil {
-		return gateway.ContentPathMetadata{}, nil, err
-	}
-	md, gr, err := blkgw.Get(ctx, path)
-	if err != nil {
-		return gateway.ContentPathMetadata{}, nil, err
-	}
-	//TODO: interfaces here aren't good enough so we're getting around the problem this way
-	runtime.SetFinalizer(gr, func(_ *gateway.GetResponse) { closeFn() })
-	return md, gr, err
-}
-
-func (api *GraphGateway) GetRange(ctx context.Context, path gateway.ImmutablePath, httpRanges ...gateway.GetRange) (gateway.ContentPathMetadata, files.File, error) {
-	rangeCount := len(httpRanges)
-	api.metrics.carParamsMetric.With(prometheus.Labels{
-		"depth": "1",
-		"bytes": fmt.Sprintf("(request ranges: %d)", rangeCount), // we dont pass specific ranges, we want deterministic number of CounterVec labels
-	}).Inc()
+func (api *GraphGateway) Get(ctx context.Context, path gateway.ImmutablePath, byteRanges ...gateway.ByteRange) (gateway.ContentPathMetadata, *gateway.GetResponse, error) {
+	rangeCount := len(byteRanges)
+	api.metrics.carParamsMetric.With(prometheus.Labels{"depth": "1", "ranges": strconv.Itoa(rangeCount)}).Inc()
 
 	carParams := "?format=car&depth=1"
 
-	// TODO: refactor this if we ever merge Get and GetRange
+	// fetch CAR with &bytes= to get minimal set of blocks for the request
 	if rangeCount > 0 {
-
 		bytesBuilder := strings.Builder{}
 		bytesBuilder.WriteString("&bytes=")
-		for i, r := range httpRanges {
+		for i, r := range byteRanges {
 			bytesBuilder.WriteString(strconv.FormatUint(r.From, 10))
 			bytesBuilder.WriteString("-")
 			if r.To != nil {
@@ -386,19 +365,18 @@ func (api *GraphGateway) GetRange(ctx context.Context, path gateway.ImmutablePat
 	if err != nil {
 		return gateway.ContentPathMetadata{}, nil, err
 	}
-	md, f, err := blkgw.GetRange(ctx, path)
-	if err != nil {
-		return gateway.ContentPathMetadata{}, nil, err
-	}
-	f, err = wrapNodeWithClose(f, closeFn)
+	md, gr, err := blkgw.Get(ctx, path, byteRanges...)
 	if err != nil {
 		return gateway.ContentPathMetadata{}, nil, err
 	}
-	return md, f, nil
+
+	//TODO: interfaces here aren't good enough so we're getting around the problem this way
+	runtime.SetFinalizer(gr, func(_ *gateway.GetResponse) { closeFn() })
+	return md, gr, nil
 }
 
 func (api *GraphGateway) GetAll(ctx context.Context, path gateway.ImmutablePath) (gateway.ContentPathMetadata, files.Node, error) {
-	api.metrics.carParamsMetric.With(prometheus.Labels{"depth": "all", "bytes": ""}).Inc()
+	api.metrics.carParamsMetric.With(prometheus.Labels{"depth": "all", "ranges": "0"}).Inc()
 	blkgw, closeFn, err := api.loadRequestIntoSharedBlockstoreAndBlocksGateway(ctx, path.String()+"?format=car&depth=all")
 	if err != nil {
 		return gateway.ContentPathMetadata{}, nil, err
@@ -415,7 +393,7 @@ func (api *GraphGateway) GetAll(ctx context.Context, path gateway.ImmutablePath)
 }
 
 func (api *GraphGateway) GetBlock(ctx context.Context, path gateway.ImmutablePath) (gateway.ContentPathMetadata, files.File, error) {
-	api.metrics.carParamsMetric.With(prometheus.Labels{"depth": "0", "bytes": ""}).Inc()
+	api.metrics.carParamsMetric.With(prometheus.Labels{"depth": "0", "ranges": "0"}).Inc()
 	// TODO: if path is `/ipfs/cid`, we should use ?format=raw
 	blkgw, closeFn, err := api.loadRequestIntoSharedBlockstoreAndBlocksGateway(ctx, path.String()+"?format=car&depth=0")
 	if err != nil {
@@ -433,10 +411,7 @@ func (api *GraphGateway) GetBlock(ctx context.Context, path gateway.ImmutablePat
 }
 
 func (api *GraphGateway) Head(ctx context.Context, path gateway.ImmutablePath) (gateway.ContentPathMetadata, files.Node, error) {
-	api.metrics.carParamsMetric.With(prometheus.Labels{
-		"depth": "",
-		"bytes": "0:1023 (Head)",
-	}).Inc()
+	api.metrics.carParamsMetric.With(prometheus.Labels{"depth": "", "ranges": "1"}).Inc()
 	blkgw, closeFn, err := api.loadRequestIntoSharedBlockstoreAndBlocksGateway(ctx, path.String()+"?format=car&bytes=0:1023")
 	if err != nil {
 		return gateway.ContentPathMetadata{}, nil, err
@@ -453,7 +428,7 @@ func (api *GraphGateway) Head(ctx context.Context, path gateway.ImmutablePath) (
 }
 
 func (api *GraphGateway) ResolvePath(ctx context.Context, path gateway.ImmutablePath) (gateway.ContentPathMetadata, error) {
-	api.metrics.carParamsMetric.With(prometheus.Labels{"depth": "0", "bytes": ""}).Inc()
+	api.metrics.carParamsMetric.With(prometheus.Labels{"depth": "0", "ranges": "0"}).Inc()
 	blkgw, closeFn, err := api.loadRequestIntoSharedBlockstoreAndBlocksGateway(ctx, path.String()+"?format=car&depth=0")
 	if err != nil {
 		return gateway.ContentPathMetadata{}, err
@@ -463,7 +438,7 @@ func (api *GraphGateway) ResolvePath(ctx context.Context, path gateway.Immutable
 }
 
 func (api *GraphGateway) GetCAR(ctx context.Context, path gateway.ImmutablePath) (gateway.ContentPathMetadata, io.ReadCloser, <-chan error, error) {
-	api.metrics.carParamsMetric.With(prometheus.Labels{"depth": "", "bytes": ""}).Inc()
+	api.metrics.carParamsMetric.With(prometheus.Labels{"depth": "", "ranges": "0"}).Inc()
 	blkgw, closeFn, err := api.loadRequestIntoSharedBlockstoreAndBlocksGateway(ctx, path.String()+"?format=car")
 	if err != nil {
 		return gateway.ContentPathMetadata{}, nil, nil, err

test-backend/handlers.go

@@ -4,6 +4,15 @@ import (
 	"bytes"
 	"context"
 	"fmt"
+	"io"
+	"net/http"
+	"net/url"
+	"runtime/debug"
+	"strconv"
+	"strings"
+	"sync"
+	"time"
+
 	"github.com/ipfs/boxo/fetcher"
 	"github.com/ipfs/boxo/files"
 	"github.com/ipfs/boxo/gateway"
@@ -13,7 +22,7 @@ import (
 	unixfile "github.com/ipfs/boxo/ipld/unixfs/file"
 	"github.com/ipfs/boxo/path"
 	"github.com/ipfs/boxo/path/resolver"
-	"github.com/ipfs/go-block-format"
+	blocks "github.com/ipfs/go-block-format"
 	format "github.com/ipfs/go-ipld-format"
 	"github.com/ipfs/go-unixfsnode"
 	dagpb "github.com/ipld/go-codec-dagpb"
@@ -23,14 +32,6 @@ import (
 	"github.com/ipld/go-ipld-prime/schema"
 	"github.com/ipld/go-ipld-prime/traversal"
 	"github.com/ipld/go-ipld-prime/traversal/selector"
-	"io"
-	"net/http"
-	"net/url"
-	"runtime/debug"
-	"strconv"
-	"strings"
-	"sync"
-	"time"
 
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
@@ -189,9 +190,9 @@ func simpleSelectorToCar(ctx context.Context, bsrv blockservice.BlockService, p
 	if hasDepth && !(depthStr == "0" || depthStr == "1" || depthStr == "all") {
 		return nil, fmt.Errorf("depth type: %s not supported", depthStr)
 	}
-	var getRange *gateway.GetRange
+	var getRange *gateway.ByteRange
 	if hasRange {
-		getRange, err = rangeStrToGetRange(rangeStr)
+		getRange, err = rangeStrToByteRange(rangeStr)
 		if err != nil {
 			return nil, err
 		}
@@ -478,7 +479,7 @@ func blockOpener(ctx context.Context, ng format.NodeGetter) ipld.BlockReadOpener
 var _ fetcher.Fetcher = (*nodeGetterFetcherSingleUseFactory)(nil)
 var _ fetcher.Factory = (*nodeGetterFetcherSingleUseFactory)(nil)
 
-func rangeStrToGetRange(rangeStr string) (*gateway.GetRange, error) {
+func rangeStrToByteRange(rangeStr string) (*gateway.ByteRange, error) {
 	rangeElems := strings.Split(rangeStr, ":")
 	if len(rangeElems) > 2 {
 		return nil, fmt.Errorf("invalid range")
@@ -489,7 +490,7 @@ func rangeStrToGetRange(rangeStr string) (*gateway.GetRange, error) {
 	}
 
 	if rangeElems[1] == "*" {
-		return &gateway.GetRange{
+		return &gateway.ByteRange{
 			From: first,
 			To:   nil,
 		}, nil
@@ -509,7 +510,7 @@ func rangeStrToGetRange(rangeStr string) (*gateway.GetRange, error) {
 		return nil, fmt.Errorf("invalid range")
 	}
 
-	return &gateway.GetRange{
+	return &gateway.ByteRange{
 		From: first,
 		To:   &second,
 	}, nil