-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow storing tags as object fields in ES - better kibana support #1018
Allow storing tags as object fields in ES - better kibana support #1018
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1018 +/- ##
=======================================
Coverage 100% 100%
=======================================
Files 139 141 +2
Lines 6419 6604 +185
=======================================
+ Hits 6419 6604 +185
Continue to review full report at Codecov.
|
6bb7f85
to
524b0a6
Compare
model/json/model.go
Outdated
Tags []KeyValue `json:"tags"` | ||
ServiceName string `json:"serviceName"` | ||
Tags []KeyValue `json:"tags"` | ||
TagsMap map[string]interface{} `json:"tagsMap,omitempty"` |
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.
Please comment if you know a better name.
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.
Ideally the name used in the kibana query would be tags
, but I guess then that may cause backward compatibility issues?
Another alternative would be to use tag
instead of tagsMap
- so when referenced it would be tag.orderId
(for example).
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.
Yes we cannot use tags
as it is used for legacy tags (as nested objects).
+1 on tag
.
dKey := strings.Replace(k, ":", ".", -1) | ||
tag := model.KeyValue{Key: dKey} | ||
// TODO v.(int/64/32) didn't work it is always a float | ||
if pInt, err := strconv.ParseInt(fmt.Sprintf("%v", v), 10, 64); err == nil { |
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.
ES always returns float64 as a number - even if it was stored as int. This works but it's not that pretty.
} | ||
|
||
// TODO we could query tagsMap only if the tag is configured | ||
// but configuration can change over time |
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 mean what if previous deployment was storing tags as object fields. If the new deployment is not using tags as fields query would not find old tags.
This is ready for review |
plugin/storage/es/options.go
Outdated
@@ -37,6 +37,9 @@ const ( | |||
suffixBulkActions = ".bulk.actions" | |||
suffixBulkFlushInterval = ".bulk.flush-interval" | |||
suffixIndexPrefix = ".index-prefix" | |||
suffixTagsFile = ".tag-file-path" | |||
suffixAllTagsAsObject = ".tag-as-fields" | |||
suffixTagDeDotChar = ".tag-key-de-dot-char" |
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.
Not sure '-de-' part is required in the name? I think tag-key-dot-char
is good enough.
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.
This is usually called de dot https://www.elastic.co/blog/introducing-the-de_dot-filter
plugin/storage/es/options.go
Outdated
@@ -75,6 +78,7 @@ func NewOptions(primaryNamespace string, otherNamespaces ...string) *Options { | |||
BulkWorkers: 1, | |||
BulkActions: 1000, | |||
BulkFlushInterval: time.Millisecond * 200, | |||
TagDeDotChar: "@", |
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.
What was the issue with using ':'?
From my ES past, I vaguely remember there being able to mark a field as not_analyzed so it was indexed but using the actual value without being processed. I see that this has been replaced by the concept of keyword in 5.x. Doesn't this avoid the issue of using '.' in the tag name?
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.
What was the issue with using ':'?
It would have to be escaped. It's used in kibana query tag.key:foo
. To avoid dots in field name we would have to set enabled:false
on the field.
I have published docker images in https://hub.docker.com/u/pavolloffay/ with tag PR1018. For example |
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.
Haven't finished yet, but wanted to share my review sooner.
if kvs == nil { | ||
kvs = make([]KeyValue, 0) | ||
} | ||
return kvs, tagsMap |
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.
If keyValues
is empty (Len=0), you'll end up with a kvs
with Len=0 and a nil tagsMap
. Wouldn't it make more sense to initialize the tagsMap
on line 89?
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.
Good question, it make sense if tagMap is used by default. By default it's not used and not present in the index.
@@ -0,0 +1,137 @@ | |||
// Copyright (c) 2017 Uber Technologies, Inc. |
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.
Copyright :)
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.
(as well as other files in this PR)
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.
what is wrong with it? It's automatically handled by make fmt
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.
s/2017 Uber Technologies, Inc/2018 The Jaeger 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.
The updadeLicense
script seems to be correct. Actually these files were copied (with changes) from domain package. Do you want to change the license?
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.
If they are just copies, then no, keep the old copyright.
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.
if with changes you can add 2018 The Jaeger Authors line above
// References and ProcessID, respectively. | ||
// When converting to ES model, ProcessID and Warnings should be omitted. Even if | ||
// included, ES with dynamic settings off will automatically ignore unneeded fields. | ||
type Span struct { |
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.
Is this struct specific to ES?
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.
yes it's in es/dbmodel package. I think the comment is not accurate. I will fix it
var expectedSpan Span | ||
require.NoError(t, json.Unmarshal(jsonStr, &expectedSpan)) | ||
|
||
testJSONEncoding(t, i, jsonStr, embeddedSpan) |
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.
Could you wrap each test in a t.Run() with an appropriate name? Like, "TestFromDomainEmbedProcess#fixture%d"
} | ||
|
||
type toDomain struct{} | ||
// ToDomain is used to convert Span to model.Span | ||
type ToDomain interface { |
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.
Same question as before: is this specific to ES? Couldn't this be made at a higher level, to be used by other storage backends? Otherwise, what's the point of changing from a struct to an interface?
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.
It is ES specific. Each storage define it's own model e.g. see C* package.
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.
The biggest advantage is keeping the struct private and exposing only the interface with builder function. It makes it clear how it is constructed.
for k, v := range tagsMap { | ||
dKey := strings.Replace(k, deDotChar, ".", -1) | ||
tag := model.KeyValue{Key: dKey} | ||
// TODO v.(int/64/32) didn't work it is always a float |
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.
What's the TODO here? Sounds more like a regular comment to me :)
if pInt, err := strconv.ParseInt(fmt.Sprintf("%v", v), 10, 64); err == nil { | ||
tag.VType = model.Int64Type | ||
tag.VInt64 = pInt | ||
} else if val, ok := v.(float64); ok { |
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.
Isn't it more idiomatic in Go to use a switch/case with the type?
switch v.(type) {
case float64:
tag.VType = model.Float64Type
tag.VFloat64 = val
...
}
} else if val, ok := v.([]byte); ok { | ||
tag.VType = model.BinaryType | ||
tag.VBinary = val | ||
} |
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.
In theory, it can also be something else. How about adding a log statement for that case?
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.
That should not really happen. In this case it's better to return an error.
s := &model.Span{Tags: []model.KeyValue{{Key: "foo", VStr: "bar"}}, | ||
Process: &model.Process{Tags: []model.KeyValue{{Key: "bar", VStr: "baz"}}}} | ||
for _, test := range testCases { | ||
mSpan := test.writer.spanConverter.FromDomainEmbedProcess(s) |
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.
Wrap every iteration with its own t.Run() with an appropriate name. In case of a failure, it should be easy to identify what's the combination that failed.
@@ -34,11 +34,18 @@ func CompareSliceOfTraces(t *testing.T, expected []*model.Trace, actual []*model | |||
} | |||
if !assert.EqualValues(t, expected, actual) { | |||
for _, err := range pretty.Diff(expected, actual) { | |||
//fmt.Println("Actual") |
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.
Will this be removed before merging?
a2be925
to
ab5fdc2
Compare
@Monnoroch @golonzovsky could you please review/run it? |
@pavolloffay as I understand it, this is backwards compatible? Regarding the CLI flags, proposal for minor adjustment:
This way if people are using config files instead of CLI then all the new options are in the same section. @black-adder can you review this? |
It is backward compatible. The proposed flags look fine except I would rename |
// v.(int/64/32) does not work. It is always a float | ||
if pInt, err := strconv.ParseInt(fmt.Sprintf("%v", v), 10, 64); err == nil { | ||
tag.VType = model.Int64Type | ||
tag.VInt64 = pInt |
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.
should use constructor functions from model
instead of direct struct manipulation, e.g.
return model.Int64(k, pint)
return kvs, nil | ||
} | ||
|
||
func convertTagField(v interface{}, k string, deDotChar string) (*model.KeyValue, error) { |
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.
return by value, no need to cause heap allocations: (model.KeyValue, error)
func SpanToDomain(span *json.Span) (*model.Span, error) { | ||
return toDomain{}.spanToDomain(span) | ||
// NewToDomain creates ToDomain | ||
func NewToDomain(deDotChar string) ToDomain { |
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.
why do we need this struct public? Would it be enough to have a public function SpanToDomain()
?
MetricsFactory metrics.Factory | ||
serviceOperationStorage *ServiceOperationStorage | ||
IndexPrefix string | ||
TagDeDotChar string |
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.
can we name this consistent with the flag, i.e. TagDotReplacement
OperationName string `json:"operationName"` | ||
References []Reference `json:"references"` | ||
StartTime uint64 `json:"startTime"` // microseconds since Unix epoch | ||
// Span adds a StartTimeMillis field to the standard JSON span. |
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.
rm this line
Tags []KeyValue `json:"tags"` | ||
Tag map[string]interface{} `json:"tag,omitempty"` | ||
Logs []Log `json:"logs"` | ||
Process *Process `json:"process,omitempty"` |
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.
any reason why this has to be a pointer and 'omitempty'? it's not optional here.
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.
good catch, we can make it mandatory it was just copied from json package.
StartTimeMillis uint64 `json:"startTimeMillis"` | ||
Duration uint64 `json:"duration"` // microseconds | ||
Tags []KeyValue `json:"tags"` | ||
Tag map[string]interface{} `json:"tag,omitempty"` |
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.
nit: I recommend moving StartTimeMillis and Tag fields to the bottom of the struct and adding comments to Tag.
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.
This affects how the json is stored to ES. I would like to keep tags
and tag
close to each other.
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.
The order the fields are defined in the Go struct are indicative of how they are stored in ES? That's good to know! Would probably be nice to have a code comment with this remark.
type Process struct { | ||
ServiceName string `json:"serviceName"` | ||
Tags []KeyValue `json:"tags"` | ||
Tag map[string]interface{} `json:"tag,omitempty"` |
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.
comment
"github.com/jaegertracing/jaeger/model" | ||
) | ||
|
||
type processHashtable struct { |
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.
when is this used? if we're embedding the Process in the Span we don't need to do deduping. Are we deduping at query time? Do we need to? I think domain-to-ui converter will dedupe anyway.
Codecov Report
@@ Coverage Diff @@
## master #1018 +/- ##
=======================================
Coverage 100% 100%
=======================================
Files 139 140 +1
Lines 6451 6620 +169
=======================================
+ Hits 6451 6620 +169
Continue to review full report at Codecov.
|
2777b56
to
a422014
Compare
can somebody re-review/approve? |
@pavolloffay |
@kacper-jackiewicz thanks for running it. It's a bug query side should use the original key format. |
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
5f34e00
to
d777e97
Compare
Can somebody re-review so it can be merged? |
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 and I think it's good to go for a RC.
plugin/storage/es/factory.go
Outdated
return nil, err | ||
} | ||
} | ||
p := esSpanStore.SpanWriterParams{Client: f.primaryClient, |
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.
No need to change IMO, but I'm wondering why did you assign it to a var instead of passing it directly to the NewSpanWriter
like you did with the other calls.
StartTimeMillis uint64 `json:"startTimeMillis"` | ||
Duration uint64 `json:"duration"` // microseconds | ||
Tags []KeyValue `json:"tags"` | ||
Tag map[string]interface{} `json:"tag,omitempty"` |
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.
The order the fields are defined in the Go struct are indicative of how they are stored in ES? That's good to know! Would probably be nice to have a code comment with this remark.
@pavolloffay can you ask someone to test with those images? Ideally existing users, as well as those who asked for the new functionality |
Hi, how can I help? We have been using the new Elasticsearch backend for ~100 days now with a fairly significant amount of span volume and lots of additional tags being added. This feature is incredibly relevant for us. |
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
@masteinhauser or anybody else could you please test this PR? Images with this functionality are available in Use |
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
I finally had some time to roll this out and test it this morning. Everything appears to be working as expected, once I actually set the correct environment variables for the collector. No other config changes appeared to be needed, although I deployed This of course only works for newly collected spans, which is perfectly acceptable for us. We'll likely be maintaining this image and deployment in production until the next Jaeger release is made with this available. Incredibly useful feature for our developers. Thank you for the work on getting this done! |
@masteinhauser thanks for trying it out! I guess we can merge it and release it. |
Client: f.primaryClient, | ||
Logger: f.logger, | ||
MetricsFactory: f.metricsFactory, | ||
MaxLookback: cfg.GetMaxSpanAge(), |
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.
@pavolloffay why did we rename it? It's confusing that config params do not match the storage params.
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 don't think we renamed anything in this PR
The flag name is --es.max-span-age
and configuration cfg.GetMaxSpanAge()
. MaxLookback
is only used in reader maybe also writer.
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 think we should change everything to maxSpanAge
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 can change it, there are also other refactoring opportunities
Resolves #906
Supersedes #980, #982
{"tag": {"http@method": "GET", "span@kind": "server"}}
.index.mapping.total_fields.limit"
it is possible to store ~940 unique span tag keys. If the limit is reached no new unique tag can be stored - ES throws an exception. It basically means span will not be stored in the current index1.0
will be always returned as int123
Stored number as interface is always a float olivere/elastic#891Introduced 3 flags:
Other changes
plugin/storage/es/spanwriter/dbmodel
Todos after this is merged:
Service
struct from writer to dbmodel packageAdding screenshot from kibana. The search pattern is
tag.span@kind:server AND process.tag.ip:10.33.144.223