Conversation
black-adder
requested review from
pavolloffay,
vprithvi and
yurishkuro
as code owners
black-adder
force-pushed
the
add_tag_filter
branch
from
0f81159 to
aab9f9a
Compare
|
I guess I need to signoff on every commit? |
black-adder
force-pushed
the
add_tag_filter
branch
from
aab9f9a to
8e5b661
Compare
ghost
assigned
black-adder
force-pushed
the
add_tag_filter
branch
from
8e5b661 to
b14c75c
Compare
| ) | ||
|
|
||
| // FilterTags returns span tags that should be inserted into cassandra. | ||
| type FilterTags func(span *model.Span) []TagInsertion |
There was a problem hiding this comment.
I am not too psyched with the data dependencies here. Filtering is unrelated to building TagInsertion.
Why not have the filter only depend on the domain model (e.g. Span and Tag for tags, and Span and Log for logs) and return true/false?
black-adder
force-pushed
the
add_tag_filter
branch
from
1ca90a2 to
48d6781
Compare
|
@caniszczyk FOSSA complains about GPL license in some of the dependencies, is that valid? Looks like 4 of them come from |
yurishkuro
left a comment
yurishkuro
left a comment
There was a problem hiding this comment.
didn't quite address my concern. The "Filter" is not just a filter, it's also a flattener, which doesn't have to be part of its function. That means we cannot chain the filters since the first one will flatten everything and deprive the next one from having access to semantics of each KeyValue. While it's a bit more work, but a cleaner model is to have independent filters for logs, tags, and process tags. It could be one interface with 3 methods, or three independent interfaces.
| import "github.com/uber/jaeger/model" | ||
|
|
||
| // FilterLogTags returns a filter that filters span.Logs. | ||
| func FilterLogTags() FilterTags { |
| spanHb.collectorOpts.WriteCacheTTL, | ||
| spanHb.metricsFactory, | ||
| spanHb.logger, | ||
| casSpanstoreModel.DefaultTagFilter(), |
There was a problem hiding this comment.
this would be better as a functional option, as the arg list is starting to get too long (and this one is clearly optional).
black-adder
force-pushed
the
add_tag_filter
branch
from
48d6781 to
7691538
Compare
yurishkuro
left a comment
yurishkuro
left a comment
There was a problem hiding this comment.
aside from one concern lgtm
| allTags = append(allTags, process.Tags...) | ||
| // GetAllUniqueTags creates a list of all unique tags from a set of filtered tags. | ||
| func GetAllUniqueTags(span *model.Span, tagFilter TagFilter) []TagInsertion { | ||
| tags := tagFilter.FilterProcessTags(span.Process.Tags) |
There was a problem hiding this comment.
- allTags was a better name imo (and would cause fewer spurious changes in this diff)
- are you sure you don't need to init to nil? if FilterProcessTags simply returns its arg, and the underlying array had extra capacity, then append might just add to that, creating potential side effects.
There was a problem hiding this comment.
Just wrote some primitive code: https://play.golang.org/p/5MbPA7Siya looks like the existing process tags doesn't get touched even if it has extra capacity
There was a problem hiding this comment.
Signed-off-by: Won Jun Jang <wjang@uber.com>
Signed-off-by: Won Jun Jang <wjang@uber.com>
Signed-off-by: Won Jun Jang <wjang@uber.com>
black-adder
force-pushed
the
add_tag_filter
branch
from
8013314 to
c73737f
Compare
* Add cassandra tag filter Signed-off-by: Won Jun Jang <wjang@uber.com>
* Add cassandra tag filter Signed-off-by: Won Jun Jang <wjang@uber.com>
3 participants
tag_filter allows the ability to decide which tags we want to index in cassandra. Some tags aren't worth indexing and this will allow you to customize which ones to filter out.