BasicHost changes for introspection #774

aarshkshah1992 · 2020-01-22T10:05:55Z

Allows the BasicHost to be "introspectable".

aarshkshah1992 · 2020-01-23T10:17:38Z

p2p/host/basic/basic_host.go

@@ -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()
+	}


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.

aarshkshah1992 · 2020-01-23T10:20:32Z

p2p/host/basic/basic_host.go

 	return h, nil
 }

+func (h *BasicHost) runtimeDataProvider() *coreit.RuntimeProviders {
+	// TODO What is the version here ?


What does version mean ?

aarshkshah1992 · 2020-01-23T10:20:47Z

p2p/host/basic/basic_host.go

@@ -783,6 +828,13 @@ func (h *BasicHost) AllAddrs() []ma.Multiaddr {
 	return dedupAddrs(finalAddrs)
 }

+// TODO Do this for routed & relay hosts as well


Need to give this a thought.

aarshkshah1992 · 2020-01-23T10:23:28Z

TODO

Release go-libp2p-core & configure the version here
Address the TODOs in code

raulk

Some initial feedback. This is shaping up good!

raulk · 2020-01-23T12:10:11Z

config/config.go

@@ -77,6 +79,9 @@ type Config struct {

 	EnableAutoRelay bool
 	StaticRelays    []peer.AddrInfo
+
+	Introspector            coreit.Introspector
+	IntrospectionServerAddr string


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.

@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 ?

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

Creating such granular field-level options muddies the namespace.

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

config/config.go

options.go

p2p/host/basic/basic_host.go

raulk

I'm gonna be refactoring things a little bit myself; no need for you to do anything here, @aarshkshah1992. Just wanting to provide feedback for future PRs.

raulk · 2020-02-06T23:38:46Z

introspect_test.go

+package libp2p
+
+import (
+	"context"


Please group imports in groups, in this order:

one group for stdlib

one group for go-libp2p-core

other go-libp2p imports

3rd party libs

raulk · 2020-02-06T23:39:41Z

introspect_test.go

+	})
+	s1, err := h1.NewStream(ctx, h3.ID(), p1)
+	require.NoError(t, err)
+	wg.Add(1)


This is racy. It's possible for wg.Done() to get called before the the wg.Add(1). Move the Add to before opening the stream to remove the race.

raulk · 2020-02-06T23:41:06Z

introspect_test.go

+	_, err = s1.Write(msg1)
+	require.NoError(t, err)
+	bz1 := make([]byte, len(msg2))
+	wg.Wait()


I don't think you need this wait at all. You can use io.ReadFull instead.

raulk · 2020-02-07T00:03:36Z

introspect_test.go

+	msg3 := []byte("111")
+	msg4 := []byte("0000")
+
+	iaddr := "0.0.0.0:9999"


Tests should not listen on 0.0.0.0 (that's insecure) nor on a fixed port (it might be occupied). Instead, we should listen on 127.0.0.1:0, and allow the listener to allocate a port. ListenAddrs() should then return the actual addresses, not the user-supplied ones.

raulk · 2020-02-07T00:04:12Z

introspect_test.go

+	bz1 := make([]byte, len(msg2))
+	wg.Wait()
+	_, err = s1.Read(bz1)
+	require.NoError(t, err)


You can do require := require.New(t) at the top, and omit the first t parameter from all assertions.

raulk · 2020-02-07T00:04:43Z

introspect_test.go

+	// host2 -> OPENS Stream 2 -> host1 , writes a message & reads the response
+	p2 := protocol.ID("h2h1")
+	h1.SetStreamHandler(p2, func(s network.Stream) {
+		bz := make([]byte, len(msg3))


Let's use buf. It shadows previous usages, and it's idiomatic.

raulk · 2020-02-07T00:05:11Z

introspect_test.go

+	_, err = s2.Write(msg3)
+	require.NoError(t, err)
+	bz2 := make([]byte, len(msg4))
+	wg.Wait()


No need for waitgroups. This code is racy, and we can use io.ReadFull instead.

raulk · 2020-02-07T00:07:04Z

introspect_test.go

+	// wait till connection is established
+	i := 0
+	var connection *websocket.Conn
+	for {


I think this loop declaration can be made more elegant, by moving the init, test, increment up here.

Or you can use https://godoc.org/github.com/stretchr/testify/require#Assertions.Eventually.

aarshkshah1992 · 2020-02-07T02:16:48Z

@raulk

Turns out I didn't push the changes after basing them on the new core changes. :/
Thanks for the review. I can make the changes myself if you like. Let me know & I'll fix the PR.

aarshkshah1992 force-pushed the feat/introspectable-host branch from 004f0e7 to 694477a Compare January 22, 2020 16:18

host changes for introspection

b5a2ce9

aarshkshah1992 force-pushed the feat/introspectable-host branch from 1c61da3 to b5a2ce9 Compare January 23, 2020 10:11

aarshkshah1992 commented Jan 23, 2020

View reviewed changes

aarshkshah1992 changed the title ~~[WIP] BasicHost changes for introspection~~ BasicHost changes for introspection Jan 23, 2020

aarshkshah1992 mentioned this pull request Jan 23, 2020

Introspector in swarm libp2p/go-libp2p-swarm#157

Merged

aarshkshah1992 changed the base branch from master to feat/introspection January 23, 2020 11:55

raulk reviewed Jan 23, 2020

View reviewed changes

aarshkshah1992 added 7 commits January 24, 2020 10:27

changes as per review

ece8853

changes as per review

ab66df8

fix import issue

1690b40

fix nil error in host

0903bf8

finished introspection changes

b81aecf

end to end tests

691c345

fix fmt

570621a

raulk requested changes Feb 7, 2020

View reviewed changes

raulk mentioned this pull request Feb 10, 2020

Introspection: constructor options and BasicHost capabilities #790

Closed

raulk merged commit 570621a into libp2p:feat/introspection Feb 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BasicHost changes for introspection #774

BasicHost changes for introspection #774

aarshkshah1992 commented Jan 22, 2020 •

edited

Loading

aarshkshah1992 Jan 23, 2020 •

edited

Loading

aarshkshah1992 Jan 23, 2020

aarshkshah1992 Jan 23, 2020

aarshkshah1992 commented Jan 23, 2020

raulk left a comment

raulk Jan 23, 2020

aarshkshah1992 Jan 24, 2020 •

edited

Loading

raulk Jan 27, 2020

raulk left a comment

raulk Feb 6, 2020

raulk Feb 6, 2020

raulk Feb 6, 2020

raulk Feb 7, 2020

raulk Feb 7, 2020

raulk Feb 7, 2020

raulk Feb 7, 2020

raulk Feb 7, 2020

raulk Feb 7, 2020

aarshkshah1992 commented Feb 7, 2020 •

edited

Loading

BasicHost changes for introspection #774

BasicHost changes for introspection #774

Conversation

aarshkshah1992 commented Jan 22, 2020 • edited Loading

aarshkshah1992 Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 commented Jan 23, 2020

TODO

raulk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 Jan 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raulk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 commented Feb 7, 2020 • edited Loading

aarshkshah1992 commented Jan 22, 2020 •

edited

Loading

aarshkshah1992 Jan 23, 2020 •

edited

Loading

aarshkshah1992 Jan 24, 2020 •

edited

Loading

aarshkshah1992 commented Feb 7, 2020 •

edited

Loading