Introspector in swarm #157

aarshkshah1992 · 2020-01-22T15:45:05Z

Changes for swarm to be able to register as a data provider & then provide the data/metrics when it is asked to do so.

raulk

Little to review here; looks sane so far.

raulk · 2020-01-23T12:36:55Z

swarm.go

@@ -9,6 +9,7 @@ import (
 	"sync/atomic"
 	"time"

+	coreit "github.com/libp2p/go-libp2p-core/introspection"


Shorten introspection to introspect, and drop the import alias.

swarm.go

aarshkshah1992 · 2020-01-27T13:26:17Z

@raulk Here are the fields that still need some discussion before we implement sourcing for them:

Stream

UserProvidedTags:
- What does this mean ?
Timeline
- How do we introspect stream.CloseTime given the fact that we remove streams from the Swarm once they are fully closed or reset. This questions ALSO applies to connection.CloseTime
- Should we use a timeCache for book-keeping non-active streams to solve this ?
Traffic
- How do we count the number of packets ?
- Is the bandwidthCounter.RateIn/Rateout a good approximation for instantaneous bandwidth ? Maybe not. What would be a good way to measure it ?
Status
- Should we add a "half-closed" status or maybe even more granular "closedRead/closedWrite" states to the introspection protocol to capture the corresponding stream states ?
- This suffers from the same problem as stream.CloseTime. Given that we remove fully closed/ resetted streams from the book-keeping, how would we show these states ?
Latency
- How do we source this ?

Connections

RelayedOver
- Need to understand how this works and where to source the Relay connection from
TransportId
- What does this represent ? Do we plan to add granular transport introspection to the protocol sometime in the future & is this the unique identifier for that ? Or does this resolve to the multiAddrCode for the transport protocol ?
Status
- Same problem as Stream.Status. How do we show states other than Open ?
Attribs
- How do we source the name of the muxer & the encryption method in the swarm ? Is it a good idea to do type assertions in the swarm by accessing the transport.CapableConn member of swarm.Conn ?
Timeline
- Outbound connections are created in Transport.Dial & upgraded by calling the upgrader from within Transport.Dial (let's assume the Transport is raw for this discussion)
- Inbound connections are created & upgraded in transport-upgrader.listener.handleIncoming
- In both cases, the swarm only ever sees the upgraded connection. How do we then source the connection creation & upgrade time in swarm ? How do we make these metrics flow from the transport/upgrader to the swarm ?

raulk

A few initial comments, mostly about style.

raulk · 2020-02-07T23:08:07Z

conn_introspect.go

+}
+
+func (ci *connIntrospector) endPoints() *introspectpb.EndpointPair {
+	return &introspectpb.EndpointPair{SrcMultiaddr: ci.c.LocalMultiaddr().String(), DstMultiaddr: ci.c.RemoteMultiaddr().String()}


nit: multi-line init is more readable in this case.

raulk · 2020-02-07T23:08:54Z

conn_introspect.go

+// TODO Number of packets & instantaneous bandwidth ?
+// Should we have a separate "flow-metre" for a connection to prepare for a message oriented world ?
+func (ci *connIntrospector) traffic() *introspectpb.Traffic {
+	if ci.s.bwc != nil {


In these cases, prefer short-circuiting to reduce one level of nesting/indentation.

if ci.s.bwc == nil { return nil } <rest>

raulk · 2020-02-07T23:09:43Z

introspect.go

+
+// IntrospectTraffic introspects & returns the overall traffic for this peer
+func (s *Swarm) IntrospectTraffic() (*introspectpb.Traffic, error) {
+	if s.bwc != nil {


Reverse the condition, and outdent the logic outside the if.

raulk · 2020-02-07T23:10:49Z

introspect.go

+		t := &introspectpb.Traffic{}
+		metrics := s.bwc.GetBandwidthTotals()
+
+		t.TrafficIn = &introspectpb.DataGauge{CumBytes: uint64(metrics.TotalIn), InstBw: uint64(metrics.RateIn)}


nit: multi-line init is more readable.

raulk · 2020-02-07T23:12:56Z

introspect.go

+	swarm.conns.RLock()
+	defer swarm.conns.RUnlock()
+
+	containsId := func(ids []introspect.ConnectionID, id string) bool {


More efficient to create a map[introspect.ConnectionID]struct{}, and make the predicate test membership. This is O(m*n) worst case scenario; map-based solution would be O(m) at the expense of a little extra memory.

raulk · 2020-02-07T23:13:38Z

introspect_test.go

+package swarm_test
+
+import (
+	"context"


Regroup imports.

raulk · 2020-02-07T23:16:12Z

swarm.go

@@ -209,11 +222,13 @@ func (s *Swarm) addConn(tc transport.CapableConn, dir network.Direction) (*Conn,
 	}

 	// Wrap and register the connection.
+	id := uuid.NewV4()


An UUID is pretty inefficient. Requires string comparison. Why not an int32 atomic counter at the swarm level, which you increment with every new connection?

aarshkshah1992 · 2020-02-09T17:02:30Z

@raulk I'm happy to refactor this PR along with implementing the stream/connection scoped metering if you like.

introspector in swarm

2669004

aarshkshah1992 force-pushed the feat/swarm-introspection branch from 68ba983 to 2669004 Compare January 23, 2020 10:28

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

raulk reviewed Jan 23, 2020

View reviewed changes

introspector for swarm

bdf15ec

aarshkshah1992 changed the title ~~Introspector in swarm~~ [WIP] Introspector in swarm Jan 24, 2020

Swarm introspection

875019a

aarshkshah1992 changed the title ~~[WIP] Introspector in swarm~~ Introspector in swarm Jan 27, 2020

aarshkshah1992 added 6 commits January 28, 2020 17:11

modified test

56ecada

introspection tests

e2c54c7

fix tests

f51a990

introspection changes as per new contract

360e973

ignore the relayed over field for now

f392174

fmt

9685707

raulk reviewed Feb 7, 2020

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introspector in swarm #157

Introspector in swarm #157

aarshkshah1992 commented Jan 22, 2020 •

edited

Loading

raulk left a comment

raulk Jan 23, 2020

aarshkshah1992 commented Jan 27, 2020 •

edited

Loading

raulk left a comment

raulk Feb 7, 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 9, 2020

Introspector in swarm #157

Introspector in swarm #157

Conversation

aarshkshah1992 commented Jan 22, 2020 • edited Loading

raulk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarshkshah1992 commented Jan 27, 2020 • edited Loading

Stream

Connections

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

aarshkshah1992 commented Feb 9, 2020

aarshkshah1992 commented Jan 22, 2020 •

edited

Loading

aarshkshah1992 commented Jan 27, 2020 •

edited

Loading