Report card fixes #583
Report card fixes #583
Conversation
Signed-off-by: Isaac Hier <isaachier@gmail.com>
Signed-off-by: Isaac Hier <isaachier@gmail.com>
isaachier
requested review from
black-adder,
pavolloffay,
vprithvi and
yurishkuro
as code owners
Signed-off-by: Isaac Hier <isaachier@gmail.com>
Signed-off-by: Isaac Hier <isaachier@gmail.com>
| @@ -53,7 +53,7 @@ func (r *Redis) FindDriverIDs(ctx context.Context, location string) []string { | |||
| span.SetTag("param.location", location) | |||
| ext.SpanKindRPCClient.Set(span) | |||
| defer span.Finish() | |||
| ctx = opentracing.ContextWithSpan(ctx, span) | |||
| _ = opentracing.ContextWithSpan(ctx, span) | |||
There was a problem hiding this comment.
This is wrong. The context is used later.
There was a problem hiding this comment.
Actually it looks like this is correct. It is not used again in the existing method and the ctx argument is copied by value (not a pointer). Unless context is an interface or something like that, cannot see how this works.
There was a problem hiding this comment.
I guess it is an interface: https://golang.org/pkg/context/#Context.
There was a problem hiding this comment.
Hard for me to verify on the phone, but isn't the context later used for logging?
storage/samplingstore/mocks/Store.go
Outdated
| @@ -19,10 +19,12 @@ import "github.com/stretchr/testify/mock" | |||
|
|
|||
| import "time" | |||
|
|
|||
| // Store writes and retrieves sampling data to and from storage. | |||
There was a problem hiding this comment.
This is generated code. It should not be edited.
pkg/distributedlock/mocks/Lock.go
Outdated
| @@ -20,10 +20,13 @@ import ( | |||
| "github.com/stretchr/testify/mock" | |||
| ) | |||
|
|
|||
| // Lock mocks distributed lock for control of a resource. | |||
model/span.go
Outdated
| @@ -96,6 +96,16 @@ func (s *Span) NormalizeTimestamps() { | |||
| } | |||
| } | |||
|
|
|||
| // FlattenTags combines span tags, process tags, and log fields into one KeyValues collection | |||
There was a problem hiding this comment.
This is used just once and the semantic value of mixing tags with log fields is highly dubious. I would avoid having this in the model.
There was a problem hiding this comment.
The only reason I extracted this as a public method was a TODO by the existing function suggesting this fix. Can easily remove.
model/keyvalue.go
Outdated
| @@ -236,6 +236,11 @@ func (kv *KeyValue) IsLess(two *KeyValue) bool { | |||
| } | |||
| } | |||
|
|
|||
| // Matches checks if this KeyValue matches the query key/value pair | |||
| func (kv *KeyValue) Matches(key, value string) bool { | |||
There was a problem hiding this comment.
Of dubious value as public method.
model/keyvalue.go
Outdated
| // FindMatch checks for a matching KeyValue in the KeyValues for the given | ||
| // query key/value pair. Returns the match and true when the match is found. | ||
| // Returns empty KeyValue and false when no match is found. | ||
| func (kvs KeyValues) FindMatch(key, value string) (KeyValue, bool) { |
There was a problem hiding this comment.
I can't see on the phone but I thought we already had a find-like method for a list of tags. Also, the method seems suspicious given that it takes bake as string.
There was a problem hiding this comment.
There was a note in the existing code explaining the FindKey method only handles one value rather than multiple.
Signed-off-by: Isaac Hier <isaachier@gmail.com>
isaachier
force-pushed
the
report-card-fixes
branch
from
6366c68
to
d9bd9f4
Compare
storage/spanstore/memory/memory.go
Outdated
| } | ||
| } | ||
| if !keyValueFoundAndMatches { | ||
| // (NB): we cannot find the KeyValues.FindKey function because there can be multiple tags with the same key |
There was a problem hiding this comment.
Yes added back note from master. Had to write local helper function instead.
There was a problem hiding this comment.
I meant changing the comment "we cannot use", not "find"
There was a problem hiding this comment.
oh lol didn't read carefully
|
Anyone want to merge this? |
Fixes #582.