v0.15.0 / Bitswap v0.9.0 make it impossible to add Tracer

[mrd0ll4r]

Checklist

• This is a bug report, not a question. Ask questions on discuss.ipfs.io.
• I have searched on the issue tracker for my bug.
• I am running the latest kubo version or have an issue updating.

Installation method

built from source

Version

git tag v0.15.0

Description

I'm maintaining the ipfs-metric-exporter plugin we use to analyze Bitswap traffic in real time.
For that, we implement PluginDaemonInternal and used to apply bitswap.WithTracer to the Bitswap instance of the running node.

The rework in Bitswap v0.9.0 has encapsulated and unexported the function-type option and made it such that this can now only be supplied to and applied by bitswap.New.
Logically, I tried to inject this option using the new fx type plugins, but to no avail.

AFAI can see, Bitswap is initialized via core.node.OnlineExchange, which does not allow me to inject any options.

What am I missing here? How can I, as a plugin author, apply the WithTracer option to the Bitswap implementation of a kubo node?

[Jorropo]

@mrd0ll4r I see this indeed is an issue.
I don't want to revert to the old behaviour, it's racy and I don't like it.

I think it make sense to add more customization inside kubo.
I don't know FX well but my guess is that we could add some optional bitswap.Option type to the OnlineExchange factory:

kubo/core/node/bitswap.go

Line 28 in 3ae52a4

return func(mctx helpers.MetricsCtx, lc fx.Lifecycle, host host.Host, rt irouting.TieredRouter, bs blockstore.GCBlockstore) exchange.Interface {

Then you could just provide thoses bitswap Options. Similar to how we do with libp2p.

As a more adhoc way you could also use the fx plugin to override the exchange factory and just add your options.

[BigLep]

@mrd0ll4r : @ajnavarro is going to link to a sample code change you could make here. After he has done so, can you please make the PR

[mrd0ll4r]

I don't want to revert to the old behaviour, it's racy and I don't like it.

I agree. It was racy and I didn't like it either :)

I don't have a strong opinion on how this is exposed, I'm just interested in it being exposed at all.

As a more adhoc way you could also use the fx plugin to override the exchange factory and just add your options.

I've been trying to achieve this for a few hours now, without luck.
I tried adding a fx.Provide with an identical signature to the one already present, i.e.

func constructModifiedBitswap() interface{} {
	return func(mctx helpers.MetricsCtx, lc fx.Lifecycle, host host.Host, rt irouting.TieredRouter, blockStore blockstore.GCBlockstore) exchange.Interface {
...
}}

But this is not allowed by fx:

Failed: cannot provide function "meplugin/metricplugin".constructModifiedBitswap.func1 (/usr/src/ipfs/metric-export-plugin/metricplugin/metric_export.go:241): cannot provide exchange.Interface from [0]: already provided by "github.com/ipfs/kubo/core/node".OnlineExchange.func1 (/usr/src/ipfs/kubo/core/node/bitswap.go:28)

I then tried to trick it by returning not just exchange.Interface, but something else I need in my plugin. I made sure the other thing is used via fx.Invoke, which should force fx to use this new constructor, but that resulted in the same problem.

Digging through the fx issues, it seems like this should be possible using Decorate.
Feeling dirty, I came up with this:

func reconstructModifiedBitswap() interface{} {
	return func(bsImpl exchange.Interface, mctx helpers.MetricsCtx, lc fx.Lifecycle, host host.Host, rt irouting.TieredRouter, blockStore blockstore.GCBlockstore) exchange.Interface {
		// Close old implementation
		if err := bsImpl.Close(); err != nil {
			panic(err) // Surely this will work
		}

                // Construct wiretap
                bitswapNetwork := bsnet.NewFromIpfsHost(host, rt)
                wiretap := NewWiretap(host.Network(), bitswapNetwork)

		// Construct new Bitswap implementation
               opts := []bs.Option{
			bs.WithTracer(wiretap),
                        // ...
		}
		exch := bs.New(helpers.LifecycleCtx(mctx, lc), bitswapNetwork, blockStore, opts...)
		lc.Append(fx.Hook{
			OnStop: func(ctx context.Context) error {
				return exch.Close()
			},
		})
		return exch
	}
}

which, to my surprise, actually compiles, gets called, and causes Bitswap to panic. 😄 (see below)
On another note: This leaves the OnStop function from the first instantiation.. somewhere. Not sure if this causes problems later one, haven't gotten that far yet. It possibly has some other side effects (like things not being cleaned up by Close, maybe, but not sure, or other things being created multiple times).

The panic:

panic: unknown option type passed to bitswap.New, got: func(*bitswap.Bitswap), 0x7f445a48fe00; expected: server.Option, client.Option or server.Option

goroutine 1 [running]:
github.com/ipfs/go-bitswap.New({0x2a52f78, 0xc002714980}, {0x2a67b00?, 0xc00272a000}, {0x7f449c51d0b8, 0xc00106a0e0}, {0xc00122f770, 0x6, 0x20?})
	/go/pkg/mod/github.com/ipfs/go-bitswap@v0.9.0/bitswap.go:76 +0xa54
meplugin/metricplugin.reconstructModifiedBitswap.func1({0x2a53218, 0xc0010542a0}, {0x7f449e74a400, 0xc0009eaba0}, {0x2a40d60?, 0xc000014380?}, {0x2a69800, 0xc0022b30c8}, {0x2a60528, 0xc0022bb920}, ...)
	/usr/src/ipfs/metric-export-plugin/metricplugin/metric_export.go:278 +0x371
...

Which reveals that a) func(bs *Bitswap) != bitswap.option (which is func(*Bitswap)) and b) the panic message is incorrect.

[ajnavarro]

we can do something similar as we did with routers here https://github.com/ipfs/kubo/blob/master/core/node/libp2p/routing.go#L58 and here https://github.com/ipfs/kubo/blob/master/core/node/libp2p/routing.go#L135 collect all bitswap.Option into a list and inject that into OnlineExange

[Jorropo]

@mrd0ll4r yeah the log message is wrong, thx 🎉 fixed it:
ipfs/go-bitswap@64bf4e9

[Jorropo]

@mrd0ll4r I actually messed up the type in go-bitswap.
I sent a pr fix here: ipfs/go-bitswap#583

[Jorropo]

I fixed it in go-bitswap v0.10.1, this will bumped in #9244

[mrd0ll4r]

we can do something similar as we did with routers here https://github.com/ipfs/kubo/blob/master/core/node/libp2p/routing.go#L58 and here https://github.com/ipfs/kubo/blob/master/core/node/libp2p/routing.go#L135 collect all bitswap.Option into a list and inject that into OnlineExange

Hmm. I've been staring at this for a while, but not too sure how it applies. I don't know fx, or how IPFS is constructed, really... Do you want me to do something like this:

1. Provide something via fx that (takes a Config and) produces []bitswap.Option, e.g.

func Foo(cfg *config.Config, provide bool) interface{} {
	return func() []bitswap.Option {
            ...
}}
...
fx.Provide(Foo(cfg,provideFlag))

3. Provide something via fx like this:

func(mctx helpers.MetricsCtx, lc fx.Lifecycle, host host.Host, rt irouting.TieredRouter, bs blockstore.GCBlockstore, opts ...bitswap.Option) exchange.Interface

And then I could fx.Decorate (or fx.Supply, adding the defaults?) the bitswap.Options with my own, and those would magically end up in the call to construct the exchange.Interface?

[ajnavarro]

@mrd0ll4r yeah, if we add opts ...bitswap.Option param to the Kubo function creating the exchange, then we can inject really easily specific options using the fx plugin depending on the use case.

[github-actions]

Oops, seems like we needed more information for this issue, please comment with more details or this issue will be closed in 7 days.

[BigLep]

I remove the stale label since a PR was submitted: #9258