all:bzzaddr instead of just overlay for logging #2000

pradovic · 2019-12-02T13:10:58Z

Trying to improve logs by printing the bzzaddr, with both overlay and underlay address, instead of just printing the overlay address, or node-id. This is attempt to solve this: #1961. It is covering mainly the network module, but there are also minor changes in the other modules as well.

In most modules, the logger is initialized with base and/or peer prefix that are used to log info about the node performing the action.

base logger:
I tried to cover all the instances of base logger and use BzzAddr here so we get both overlay/underlay pair in logs.

peer logger:
The same rationale is used as for base, however since the overlay address is not accurate for peers without the bzz-handshake I don't see a way to log this information correctly outside of the, mostly, network package.

Questions:

Is this needed for base?
Do we need to print both overlay and underlay instead of peer.ID on all places? (ex. r.logger.Trace("findpeer skip peer", "peer", id, "ref", req.Addr.String()))

Improvement:

It would be nice to figure out the way to exclude base logs in production and include it only in tests.

janos · 2019-12-02T13:39:49Z

Thanks Petar.

Is this needed for base?

I would assume that base address in this context is BzzAddr of the running node itself, not its peers it is connected to. In that case, node addresses are not needed in the production, but that information is useful when debugging test simulations, where we have printed logs from all nodes in the single output.

Do we need to print both overlay and underlay instead of peer.ID on all places? (ex. r.logger.Trace("findpeer skip peer", "peer", id, "ref", req.Addr.String()))

I believe that this would improve information that are relevant for debugging.

pradovic · 2019-12-02T13:42:52Z

Thanks Janos. Cool, I will leave the short address in the base as well, and check if I can add it wherever peer id is used, if possible. At least in network module.

janos · 2019-12-02T13:59:37Z

storage/netstore.go

@@ -92,14 +94,14 @@ type NetStore struct {
 }

 // NewNetStore creates a new NetStore using the provided chunk.Store and localID of the node.
-func NewNetStore(store chunk.Store, baseAddr []byte, localID enode.ID) *NetStore {
+func NewNetStore(store chunk.Store, baseAddr *network.BzzAddr, localID enode.ID) *NetStore {


BzzAddr already provides enode.ID with its ID() method, so localID should not be needed.

janos · 2019-12-02T14:09:08Z

network/stream/peer.go

-// NewPeer is the constructor for Peer
-func NewPeer(peer *network.BzzPeer, baseKey []byte, i state.Store, providers map[string]StreamProvider) *Peer {
+// newPeer is the constructor for Peer
+func newPeer(peer *network.BzzPeer, address *network.BzzAddr, i state.Store, providers map[string]StreamProvider) *Peer {


BzzPeer already has BzzAddr. Maybe it is possible to remove address argument?

If I am not mistaken, peer has a BzzAddr for a peer, and address is a base address. I will rename the argument to baseAddress to avoid the confusion.

janos · 2019-12-02T14:11:39Z

network/retrieval/retrieve_test.go

@@ -621,7 +621,7 @@ func newRetrievalTester(t *testing.T, prvkey *ecdsa.PrivateKey, netStore *storag
 		prvkey = key
 	}

-	r := New(kad, netStore, kad.BaseAddr(), nil)
+	r := New(kad, netStore, network.NewBzzAddr(kad.BaseAddr(), network.PrivateKeyToBzzKey(prvkey)), nil)


PrivateKeyToBzzKey is creating an Overlay address, but here is used in place of Underlay address. Is it correct to set it to nil as it is not available in this function and not used in these tests?

Yup, I think we can set it to nil as well.

network/network.go

janos · 2019-12-02T16:54:02Z

api/inspector_test.go

@@ -33,16 +32,17 @@ func TestInspectorPeerStreams(t *testing.T) {
 		t.Fatal(err)
 	}

+	baseAddress := network.NewBzzAddr(baseKey, baseKey)


Just an idea, for the underlay address, to use nil instead, not to present an example where overlay address is used as a value from underlay?

The same question stands for other places where NewBzzAddr is used with identical value for arguments.

I agree, I started to change this on the whole project but did not push yet :) Thanks!

Actually, this concrete case needs underlay address because of the PeerInfo() function, so I have decided to create another rand key for peer base address. I know it's not needed, but I think it gives more info in tests about the nature of address. WDYT?

Or maybe just to add a comment justifying the usage of baseKey in this particular case. That would be also good, at least for me.

janos · 2019-12-02T16:55:20Z

api/inspector_test.go


-	i := NewInspector(nil, nil, netStore, stream.New(state.NewInmemoryStore(), baseKey, stream.NewSyncProvider(netStore, network.NewKademlia(
+	i := NewInspector(nil, nil, netStore, stream.New(state.NewInmemoryStore(), network.NewBzzAddr(baseKey, baseKey), stream.NewSyncProvider(netStore, network.NewKademlia(


baseAddress instead NewBzzAddr?

janos · 2019-12-02T17:00:48Z

swarm.go

-	self.netStore = storage.NewNetStore(lstore, bzzconfig.OverlayAddr, nodeID)
-	self.retrieval = retrieval.New(to, self.netStore, bzzconfig.OverlayAddr, self.swap) // nodeID.Bytes())
+	self.netStore = storage.NewNetStore(lstore, bzzconfig.Address)
+	self.retrieval = retrieval.New(to, self.netStore, bzzconfig.Address, self.swap) // nodeID.Bytes())


Remove commented code.

Ok, it was there before, so I did not erase it yet.

Yes, I did not notice it, thanks.

zelig · 2019-12-04T05:37:51Z

network/network.go

+	return fmt.Sprintf("%s:%s", a.ShortOver(), a.ShortUnder())
+}
+
+func (a *BzzAddr) ShortOver() string {


comment exported functions

janos · 2019-12-04T10:07:58Z

network/network.go

+	return fmt.Sprintf("%s:%s", a.ShortOver(), a.ShortUnder())
+}
+
+// ShortOver prints shortened version of Overlay address


prints -> returns

use bzzaddr instead of just overlay for logging

d7beb3a

pradovic added the in progress label Dec 2, 2019

pradovic requested a review from janos December 2, 2019 13:10

pradovic self-assigned this Dec 2, 2019

pradovic added 2 commits December 2, 2019 14:19

go fmt

f555d0a

fixed linting errors

492493f

janos reviewed Dec 2, 2019

View reviewed changes

pradovic added 5 commits December 2, 2019 15:50

handle nil and bad values in network.ShortString()

a956e63

ommit unnecessary slicing

cb0751b

added use of bzzaddr for logging wherever easily possible

f9317e8

go fmt

b243efe

fix linting

f4d2c61

janos reviewed Dec 2, 2019

View reviewed changes

pradovic added 5 commits December 2, 2019 20:16

fixed bzzaddr test intialization

09aa7b5

fixed inspector tests

d1c9387

fixed inspector properly

9faf4cd

added comments

0343a0d

minor test fixes

5cb4bf3

pradovic changed the title ~~network:bzzaddr instead of just overlay for logging~~ all:bzzaddr instead of just overlay for logging Dec 3, 2019

pradovic added 2 commits December 3, 2019 13:13

few more minor logs change

afb712e

minor cosmetic changes

1164e20

pradovic requested a review from zelig December 3, 2019 15:20

pradovic added ready for review and removed in progress labels Dec 3, 2019

zelig approved these changes Dec 4, 2019

View reviewed changes

comments fix

45c2834

janos reviewed Dec 4, 2019

View reviewed changes

janos approved these changes Dec 4, 2019

View reviewed changes

pradovic added 2 commits December 4, 2019 12:24

minor comments fix

a5b0aeb

merge with master

1613806

pradovic merged commit 45a40fc into master Dec 4, 2019

pradovic deleted the oaddr-logs-improve branch December 5, 2019 10:52

acud added this to the 0.5.5 milestone Jan 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

all:bzzaddr instead of just overlay for logging #2000

all:bzzaddr instead of just overlay for logging #2000

pradovic commented Dec 2, 2019 •

edited

Loading

janos commented Dec 2, 2019

pradovic commented Dec 2, 2019

janos Dec 2, 2019

janos Dec 2, 2019

pradovic Dec 2, 2019

janos Dec 2, 2019

pradovic Dec 2, 2019

janos Dec 2, 2019

pradovic Dec 2, 2019

pradovic Dec 2, 2019

janos Dec 2, 2019

janos Dec 2, 2019

janos Dec 2, 2019

pradovic Dec 2, 2019

janos Dec 2, 2019

zelig Dec 4, 2019

janos Dec 4, 2019


		i := NewInspector(nil, nil, netStore, stream.New(state.NewInmemoryStore(), baseKey, stream.NewSyncProvider(netStore, network.NewKademlia(
		i := NewInspector(nil, nil, netStore, stream.New(state.NewInmemoryStore(), network.NewBzzAddr(baseKey, baseKey), stream.NewSyncProvider(netStore, network.NewKademlia(

all:bzzaddr instead of just overlay for logging #2000

all:bzzaddr instead of just overlay for logging #2000

Conversation

pradovic commented Dec 2, 2019 • edited Loading

janos commented Dec 2, 2019

pradovic commented Dec 2, 2019

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

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

pradovic commented Dec 2, 2019 •

edited

Loading