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.

Sign up for GitHub

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

BasicHost changes for introspection #774

Merged
merged 8 commits into from
Feb 10, 2020
Merged

BasicHost changes for introspection #774

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b5a2ce9
host changes for introspection
aarshkshah1992 Jan 22, 2020
ece8853
changes as per review
aarshkshah1992 Jan 24, 2020
ab66df8
changes as per review
aarshkshah1992 Jan 24, 2020
1690b40
fix import issue
aarshkshah1992 Jan 28, 2020
0903bf8
fix nil error in host
aarshkshah1992 Jan 28, 2020
b81aecf
finished introspection changes
aarshkshah1992 Jan 28, 2020
691c345
end to end tests
aarshkshah1992 Jan 29, 2020
570621a
fix fmt
aarshkshah1992 Jan 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 19 additions & 6 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@ import (
"github.com/libp2p/go-libp2p-core/connmgr"
"github.com/libp2p/go-libp2p-core/crypto"
"github.com/libp2p/go-libp2p-core/host"
coreit "github.com/libp2p/go-libp2p-core/introspection"
raulk marked this conversation as resolved.
Show resolved Hide resolved
"github.com/libp2p/go-libp2p-core/metrics"
"github.com/libp2p/go-libp2p-core/network"
"github.com/libp2p/go-libp2p-core/peer"
"github.com/libp2p/go-libp2p-core/peerstore"
"github.com/libp2p/go-libp2p-core/pnet"
"github.com/libp2p/go-libp2p-core/routing"

"github.com/libp2p/go-libp2p-introspection/introspection"
bhost "github.com/libp2p/go-libp2p/p2p/host/basic"
relay "github.com/libp2p/go-libp2p/p2p/host/relay"
routed "github.com/libp2p/go-libp2p/p2p/host/routed"
Expand Down Expand Up @@ -77,6 +79,9 @@ type Config struct {

EnableAutoRelay bool
StaticRelays []peer.AddrInfo

Introspector coreit.Introspector
IntrospectionServerAddr string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should allow the user to pass in an address. For UX, I would recommend the following:

  • We add an ListenAddress() string method to the Introspector interface, so we can query it later.
  • The Introspector implementation has a DefaultAddress, and may use fallback logic if the default listen address is taken. For example, the default address could be: ws://localhost:8096, and if the port is occupied, we can fall back to trying 8096 + [1,100], trying one by one until we find an available port, or fail entirely. See https://github.com/libp2p/go-libp2p-daemon/blob/b95e77dbfcd186ccf817f51e95f73f9fd5982600/p2pd/main.go#L30-L60 for an example of this logic.
  • We expose the final listen address via the ListenAddress() string method.
  • Pushing this concern to the implementation is better, because alternate implementations may use different protocols (e.g. Unix sockets) and heuristics.

For a user that wants to set a concrete address, they would construct the concrete implementation via its constructor (which would take a configuration object, that includes the listen addr), and inject that via the Introspector option.

Copy link
Contributor Author

@aarshkshah1992 aarshkshah1992 Jan 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@raulk

I am not sure I agree. Why shouldn't we allow to user to pass in a address while creating a libp2p node ?

The address is a required by the introspection WS server which is owned by the Host.

The Introspector is only responsible for proving a registry for sub-systems & resolving the state when it is asked to. It is not/shouldn't be responsible for serving that state over the wire & hence dosen't need to be bothered with finding a free port which is anyways not a trivial operation & muddles the construction logic of the Introspector.

I'd like to make ListenAddress() string a part of the IntrospectableHost interface that you've described here. The go-libp2p-introspection.StartWsServer method returns the address it was able to bind to(using the same logic as the code that you've linked) which the host then captures & returns via ListenAddress() string.

Wdyt ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I was unclear. We should totally allow passing in a listen address, but not as a dedicated host option. For two reasons:

  1. Creating such granular field-level options muddies the namespace.
  2. Listen addresses, along with all potential customisations of the introspection endpoint, are implementation-dependent.

Users wanting to customise the listen addr of an introspection server, should be able to do that through the endpoint's constructor:

ep, err := introspection.Endpoint(&introspection.EndpointOpts{
    ListenAddress: "...",
}
h, err := host.New(host.Introspector(ep))

}

// NewNode constructs a new libp2p Host from the Config.
Expand Down Expand Up @@ -114,18 +119,25 @@ func (cfg *Config) NewNode(ctx context.Context) (host.Host, error) {
return nil, err
}

introspector := cfg.Introspector
if introspector == nil {
introspector = introspection.NewDefaultIntrospector()
}

// TODO: Make the swarm implementation configurable.
swrm := swarm.NewSwarm(ctx, pid, cfg.Peerstore, cfg.Reporter)
swrm := swarm.NewSwarm(ctx, pid, cfg.Peerstore, cfg.Reporter, introspector)
if cfg.Filters != nil {
swrm.Filters = cfg.Filters
}

h, err := bhost.NewHost(ctx, swrm, &bhost.HostOpts{
ConnManager: cfg.ConnManager,
AddrsFactory: cfg.AddrsFactory,
NATManager: cfg.NATManager,
EnablePing: !cfg.DisablePing,
UserAgent: cfg.UserAgent,
ConnManager: cfg.ConnManager,
AddrsFactory: cfg.AddrsFactory,
NATManager: cfg.NATManager,
EnablePing: !cfg.DisablePing,
UserAgent: cfg.UserAgent,
Introspector: introspector,
IntrospectionServerAddr: cfg.IntrospectionServerAddr,
})

if err != nil {
Expand Down Expand Up @@ -242,6 +254,7 @@ func (cfg *Config) NewNode(ctx context.Context) (host.Host, error) {
if router != nil {
return routed.Wrap(h, router), nil
}

return h, nil
}

Expand Down
23 changes: 23 additions & 0 deletions options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package libp2p

import (
"fmt"
coreit "github.com/libp2p/go-libp2p-core/introspection"
raulk marked this conversation as resolved.
Show resolved Hide resolved
"net"

"github.com/libp2p/go-libp2p-core/connmgr"
Expand Down Expand Up @@ -193,6 +194,28 @@ func ConnectionManager(connman connmgr.ConnManager) Option {
}
}

// Introspector configures the host to use the given introspector
func Introspector(i coreit.Introspector) Option {
return func(cfg *Config) error {
if cfg.Introspector != nil {
return fmt.Errorf("cannot specify multiple introspectors")
}
cfg.Introspector = i
return nil
}
}

// IntrospectionServerAddr configures the address for the introspection server
func IntrospectionServerAddr(addr string) Option {
return func(cfg *Config) error {
if len(cfg.IntrospectionServerAddr) != 0 {
return fmt.Errorf("cannot specify multiple introspection server addresses")
}
cfg.IntrospectionServerAddr = addr
return nil
}
}

// AddrsFactory configures libp2p to use the given address factory.
func AddrsFactory(factory config.AddrsFactory) Option {
return func(cfg *Config) error {
Expand Down
76 changes: 64 additions & 12 deletions p2p/host/basic/basic_host.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,23 @@ import (
"context"
"io"
"net"
runtime2 "runtime"
"sync"
"time"

"github.com/libp2p/go-libp2p-introspection/introspection"
"github.com/libp2p/go-libp2p/p2p/protocol/identify"
"github.com/libp2p/go-libp2p/p2p/protocol/ping"

"github.com/libp2p/go-libp2p-core/connmgr"
"github.com/libp2p/go-libp2p-core/event"
"github.com/libp2p/go-libp2p-core/host"
coreit "github.com/libp2p/go-libp2p-core/introspection"
"github.com/libp2p/go-libp2p-core/network"
"github.com/libp2p/go-libp2p-core/peer"
"github.com/libp2p/go-libp2p-core/peerstore"
"github.com/libp2p/go-libp2p-core/protocol"

"github.com/libp2p/go-eventbus"
inat "github.com/libp2p/go-libp2p-nat"

logging "github.com/ipfs/go-log"
Expand All @@ -35,6 +37,8 @@ import (
// peer (for all addresses).
const maxAddressResolution = 32

var _ coreit.IntrospectorRegistry = (*BasicHost)(nil)

var log = logging.Logger("basichost")

var (
Expand Down Expand Up @@ -89,6 +93,9 @@ type BasicHost struct {
emitters struct {
evtLocalProtocolsUpdated event.Emitter
}

// introspector is the host introspector used to register subsystem metric providers & fetch the current system state when asked
introspector coreit.Introspector
}

var _ host.Host = (*BasicHost)(nil)
Expand Down Expand Up @@ -124,6 +131,12 @@ type HostOpts struct {

// UserAgent sets the user-agent for the host. Defaults to ClientVersion.
UserAgent string

// Introspector is used by host subsystems to register themselves as metrics providers & fetch the current state.
Introspector coreit.Introspector

// IntrospectionServerAddr is the address for the introspection server
IntrospectionServerAddr string
}

// NewHost constructs a new *BasicHost and activates it by attaching its stream and connection handlers to the given inet.Network.
Expand All @@ -135,24 +148,14 @@ func NewHost(ctx context.Context, net network.Network, opts *HostOpts) (*BasicHo
AddrsFactory: DefaultAddrsFactory,
maResolver: madns.DefaultResolver,
eventbus: eventbus.NewBus(),
introspector: opts.Introspector,
}

var err error
if h.emitters.evtLocalProtocolsUpdated, err = h.eventbus.Emitter(&event.EvtLocalProtocolsUpdated{}); err != nil {
return nil, err
}

h.proc = goprocessctx.WithContextAndTeardown(ctx, func() error {
if h.natmgr != nil {
h.natmgr.Close()
}
if h.cmgr != nil {
h.cmgr.Close()
}
_ = h.emitters.evtLocalProtocolsUpdated.Close()
return h.Network().Close()
})

if opts.MultistreamMuxer != nil {
h.mux = opts.MultistreamMuxer
}
Expand Down Expand Up @@ -191,12 +194,54 @@ func NewHost(ctx context.Context, net network.Network, opts *HostOpts) (*BasicHo
h.pings = ping.NewPingService(h)
}

if h.introspector == nil {
h.introspector = introspection.NewDefaultIntrospector()
}
Copy link
Contributor Author

@aarshkshah1992 aarshkshah1992 Jan 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This duplicates the code in config.NewNode() here.

We can safely assume here that an introspector exists.

However, do we want to allow users to be able to disable introspection ? That decision will affect how we've currently configured introspection on the host.


net.SetConnHandler(h.newConnHandler)
net.SetStreamHandler(h.newStreamHandler)

// register runtime provider
if err := h.introspector.RegisterProviders(&coreit.ProvidersTree{Runtime: h.runtimeDataProvider()}); err != nil {
log.Errorf("failed to register a runtime provider, err=%s", err)
}

// start introspection server
// TODO What happens if address is not configured or not available ?
// TODO How do we configure a "default address"
shutDownFnc := introspection.StartServer(opts.IntrospectionServerAddr, h.introspector)

h.proc = goprocessctx.WithContextAndTeardown(ctx, func() error {
if h.natmgr != nil {
h.natmgr.Close()
}
if h.cmgr != nil {
h.cmgr.Close()
}
_ = h.emitters.evtLocalProtocolsUpdated.Close()

if err := shutDownFnc(); err != nil {
log.Errorf("error while shutting down introspection server, err=%s", err)
}

return h.Network().Close()
})

return h, nil
}

func (h *BasicHost) runtimeDataProvider() *coreit.RuntimeProviders {
// TODO What is the version here ?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does version mean ?

runtime := &coreit.Runtime{Implementation: "go-libp2p",
Platform: runtime2.GOOS,
PeerId: h.ID().Pretty(),
}

return &coreit.RuntimeProviders{Get: func() (*coreit.Runtime, error) {
return runtime, nil
}}
}

// New constructs and sets up a new *BasicHost with given Network and options.
// The following options can be passed:
// * NATPortMap
Expand Down Expand Up @@ -783,6 +828,13 @@ func (h *BasicHost) AllAddrs() []ma.Multiaddr {
return dedupAddrs(finalAddrs)
}

// TODO Do this for routed & relay hosts as well
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to give this a thought.

// RegisterProvider allows the host to be an IntrospectorRegistry. Please look at the docs for the IntrospectorRegistry
// interface in go-libp2p-core for more details.
func (h *BasicHost) RegisterProviders(p *coreit.ProvidersTree) error {
raulk marked this conversation as resolved.
Show resolved Hide resolved
return h.introspector.RegisterProviders(p)
}

// Close shuts down the Host's services (network, etc).
func (h *BasicHost) Close() error {
// You're thinking of adding some teardown logic here, right? Well
Expand Down