-
Notifications
You must be signed in to change notification settings - Fork 110
Conversation
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.
Basically looks good to me, just a couple of observations we can discuss about
swarm/api/client/client.go
Outdated
@@ -489,6 +502,17 @@ func (c *Client) TarUpload(hash string, uploader Uploader, defaultPath string, t | |||
if err != nil { | |||
return "", err | |||
} | |||
|
|||
opentracing.GlobalTracer().Inject( |
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.
@justelad I don't like this, because users might also use this client - we don't want opentracing
headers to be added always.
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.
I suggest we get rid of this opentracing
from here, so that we can merge this PR as it is, because it is getting large.
swarm/api/client/client.go
Outdated
opentracing.HTTPHeaders, | ||
opentracing.HTTPHeadersCarrier(req.Header)) | ||
|
||
trace := GetClientTrace("swarm api client - upload tar", "swarm.api.client.uploadtar", uuid.New()[:8], &tn) |
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
. We are adding the prefix as part of the reporter.
swarm/api/client/client.go
Outdated
@@ -474,6 +482,11 @@ type UploadFn func(file *File) error | |||
// TarUpload uses the given Uploader to upload files to swarm as a tar stream, | |||
// returning the resulting manifest hash | |||
func (c *Client) TarUpload(hash string, uploader Uploader, defaultPath string, toEncrypt bool) (string, error) { | |||
ctx, sp := spancontext.StartSpan(context.Background(), "swarm.api.client.tarupload") |
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
.
@@ -728,3 +752,57 @@ func (c *Client) GetFeedRequest(query *feed.Query, manifestAddressOrDomain strin | |||
} | |||
return &metadata, nil | |||
} | |||
|
|||
func GetClientTrace(traceMsg, metricPrefix, ruid string, tn *time.Time) *httptrace.ClientTrace { |
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 fetch
and clienttrace
prefixes - these can be added to metricsPrefix
if needed.
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 if @nonsense comment's addressed
No description provided.