Save baggage in span #153
Save baggage in span #153
Conversation
|
I think we can simply log it, to avoid introducing a new data model concept, as I don't see a really strong use case for it being a first class citizen. The log would be structured as event=baggage |
|
sure, a little extra work on the jaeger model to ui model conversion then |
|
UI can easily find the logs with baggage event. Again, let's try to not change the data models if we don't have to. |
span_test.go
Outdated
| value := logRecord.Fields[2].Value().(string) | ||
|
|
||
| require.Contains(t, expected, key) | ||
| assert.Equal(t, value, expected[key]) |
span.go
Outdated
| @@ -173,6 +178,15 @@ func (s *Span) SetBaggageItem(key, value string) opentracing.Span { | |||
| s.Lock() | |||
| defer s.Unlock() | |||
| s.context = s.context.WithBaggageItem(key, value) | |||
| if !s.context.IsSampled() { | |||
| return s | |||
There was a problem hiding this comment.
nit: this shortcut makes the assumption that the baggage log is always the last thing, which may not be the correct assumption in the future. It's better to include the log statement inside the if - it does not introduce significant nesting.
There was a problem hiding this comment.
not completely following, if the span is not sampled, we shouldn't log the span so it should fall outside of the id stmt. Not sure what you mean by "the baggage log is always the last thing"
There was a problem hiding this comment.
I simply meant
If sampled then log baggage.
span.go
Outdated
| } | ||
|
|
||
| // this function should only be called while holding a Write lock | ||
| func (s *Span) logFieldsWithLock(fields ...log.Field) { |
There was a problem hiding this comment.
nit: L100: setTagNoLocking, here WithLock. We should be consistent.
propagation_test.go
Outdated
| @@ -104,7 +104,7 @@ func TestSpanPropagator(t *testing.T) { | |||
| assert.Equal(t, exp.context, sp.context, formatName) | |||
| assert.Equal(t, "span.kind", sp.tags[0].key) | |||
| assert.Equal(t, expTags, sp.tags[1:] /*skip span.kind tag*/, formatName) | |||
| assert.Equal(t, exp.logs, sp.logs, formatName) | |||
| assert.NotEqual(t, exp.logs, sp.logs, formatName) // Only the parent span should have baggage logs | |||
There was a problem hiding this comment.
Not equal is a very weak test, it may succeed for other reasons
We want to save the baggage when it is first set within the span. I think it makes sense to make baggage a first class item.
The new jaeger.thrift idl will look like:
Alternatives would be either tags or logs.
tag: we only have key-value so it would have to be a json blob of baggage:{key, value, timestamp}
log: same as above, we would have to use the fields which are key-value as well (json blob)
By making this a first class citizen, we don't need to manually parse the tags/logs for the UI.
Why timestamp in BaggageRecord? If the same baggage key is set multiple times, we can use the timestamp to differentiate them. (Not sure if it's really needed)
Fixes https://github.com/uber/jaeger/issues/179