-
Notifications
You must be signed in to change notification settings - Fork 110
swarm/network: Keep span across roundtrip #1210
Conversation
6d64161
to
46525b3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should document this code more?
This corrects the previous change which broke the build and was pushed by accident.
This changes default location of the data directory to use the LOCALAPPDATA environment variable, resolving issues with remote home directories an improving compatibility with Cygwin. Fixes #2239 Fixes #2237 Fixes #16437
Package crypto works with or without cgo, which is great. However, to make it work without cgo required setting the build tag `nocgo`. It's common to disable cgo by instead just setting the environment variable `CGO_ENABLED=0`. Setting this environment variable does _not_ implicitly set the build tag `nocgo`. So projects that try to build the crypto package with `CGO_ENABLED=0` will fail. I have done this myself several times. Until today, I had just assumed that this meant that this package requires cgo. But a small build tag change will make this case work. Instead of using `nocgo` and `!nocgo`, we can use `!cgo` and `cgo`, respectively. The `cgo` build tag is automatically set if cgo is enabled, and unset if it is disabled.
Simplifies the transaction presense check to use a function to determine if the transaction is present in the block provided to trace, which originally had a redundant parenthesis and used a `exist` flag to dictate control flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but please fix the few minor comments before submitting upstream <3
swarm/network/stream/peer.go
Outdated
@@ -83,16 +84,11 @@ func NewPeer(peer *protocols.Peer, streamer *Registry) *Peer { | |||
clients: make(map[Stream]*client), | |||
clientParams: make(map[Stream]*clientParams), | |||
quit: make(chan struct{}), | |||
spans: sync.Map{}, | |||
//spans: sync.Map{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
swarm/network/stream/stream.go
Outdated
@@ -884,6 +887,10 @@ func (r *Registry) Start(server *p2p.Server) error { | |||
} | |||
|
|||
func (r *Registry) Stop() error { | |||
r.spans.Range(func(k, v interface{}) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_, v interface{})
?
swarm/storage/netstore.go
Outdated
@@ -1,4 +1,4 @@ | |||
// Copyright 2016 The go-ethereum Authors | |||
//// Copyright 2016 The go-ethereum Authors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//
swarm/tracing/tracing.go
Outdated
// FinishSpans calls `Finish()` on all stored spans | ||
// It should be called on instance shutdown | ||
func FinishSpans() { | ||
store.spans.Range(func(k, v interface{}) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_,v
49c40ba
to
f0a4c63
Compare
submitted upstream ethereum/go-ethereum#19140 |
This PR addresses #1209 (see copy of description below) which is an incremental part of #1181
Until now, the spans are only valid within internal peer contexts, but we may want to trace durations from node makes request to node received delivery.
According to
opentracing
support, if aSpan
is to persist across asynchronous operations, it's the requester's responsibility to remember theSpan
andFinish()
it accordingly. In other words, aSpan
cannot be recalled after passing through thetracer.Inject()
/tracer.Extract()
serialization, only new spans (ChildOf()
,FollowFrom()
) can be created from theSpanContext
that's passed on.To this end, I've introduced a "store" for spans to be remembered. This "store" is a singleton per swarm instance. It seems to me to belong in the
swarm/tracing
package, so I put it there.Unlike
LazyChunkReader.Read
, we don't know how long a singleRetrieve Request
took. We should update the spans to record the full duration of a retrieve request, until it is actually delivered on the requestor node.Good
Bad