Adjust to the OpenTracing key-value logging change #25
Conversation
| @@ -19,7 +19,12 @@ type EventBaggage struct { | |||
| Key, Value string | |||
| } | |||
|
|
|||
| // EventLogFields is received when LogFields or LogKV is called. | |||
| type EventLogFields opentracing.LogRecord | |||
There was a problem hiding this comment.
(I mirrored changes to basictracer-go here and elsewhere)
| @@ -116,7 +142,7 @@ func (s *spanImpl) Log(ld opentracing.LogData) { | |||
| ld.Timestamp = time.Now() | |||
| } | |||
|
|
|||
| s.raw.Logs = append(s.raw.Logs, ld) | |||
| s.raw.Logs = append(s.raw.Logs, ld.ToLogRecord()) | |||
There was a problem hiding this comment.
We use a helper to convert old stuff to new stuff.
| @@ -107,8 +107,9 @@ func TestSpan_SingleLoggedTaggedSpan(t *testing.T) { | |||
| assert.Equal(t, 1, len(spans)) | |||
| assert.Equal(t, "x", spans[0].Operation) | |||
| assert.Equal(t, 1, len(spans[0].Logs)) | |||
| assert.Equal(t, "event", spans[0].Logs[0].Event) | |||
| assert.Equal(t, "payload", spans[0].Logs[0].Payload) | |||
| // XXX: broken tests | |||
There was a problem hiding this comment.
I didn't want to fix these until we decided what to do about the annotations more generally.
| spLog.Timestamp = time.Now() | ||
| } | ||
| annotate(span, spLog.Timestamp, spLog.Event, r.endpoint) | ||
| // XXX: decide how to represent spLog.Fields as an annotation |
There was a problem hiding this comment.
@basvanbeek @adriancole this is the only interesting bit... how much / how little of the key:value data do you want to represent as timestamped zipkin Span annotations? If you tell me what you'd like this to be, I'm happy to write up an Encoder impl, it'll just take a few minutes.
There was a problem hiding this comment.
heh probably not the best one to comment, as I've never been a fan of this feature. it is a complex datastructure (map) with a timestamp. IMHO, this sort of thing is a front-end rendering or layout problem, in the same space as things like log4j, unless we are defining this as a backend/wire format concern.
Before I speculate how this might be used in zipkin, it is probably best to have the champion of this feature comment. @yurishkuro are you planning to make an encoder on the frontend? or are you developing a non-zipkin backend data structure to replace zipkin? or do you have other plans wrt this thing?
Until then, here's a hint.. Let's assume we didn't assume responsibilities like normal loggers have with regards to formatting, and we just did json encoding.. ex {"event": "soft error", "type": "cache timeout", "waited.millis": 1500}
With no changes to the existing UI or data format, it ends up looking like this (the mis-rendering is probably a bug):
And here's it in the up-and-coming UI by @rogeralsing
| @@ -91,6 +92,31 @@ func (s *spanImpl) SetTag(key string, value interface{}) opentracing.Span { | |||
| return s | |||
| } | |||
|
|
|||
| func (s *spanImpl) LogKV(keyValues ...interface{}) { | |||
There was a problem hiding this comment.
just to be clear... KV is a required part of the api now?
There was a problem hiding this comment.
Yeah, this dates back to opentracing/opentracing.io#96 .
The use cases haven't been well-centralized, but I feel good about having a timestamped KV at the bottom.
| spLog.Timestamp = time.Now() | ||
| } | ||
| annotate(span, spLog.Timestamp, spLog.Event, r.endpoint) | ||
| // XXX: decide how to represent spLog.Fields as an annotation |
There was a problem hiding this comment.
heh probably not the best one to comment, as I've never been a fan of this feature. it is a complex datastructure (map) with a timestamp. IMHO, this sort of thing is a front-end rendering or layout problem, in the same space as things like log4j, unless we are defining this as a backend/wire format concern.
Before I speculate how this might be used in zipkin, it is probably best to have the champion of this feature comment. @yurishkuro are you planning to make an encoder on the frontend? or are you developing a non-zipkin backend data structure to replace zipkin? or do you have other plans wrt this thing?
Until then, here's a hint.. Let's assume we didn't assume responsibilities like normal loggers have with regards to formatting, and we just did json encoding.. ex {"event": "soft error", "type": "cache timeout", "waited.millis": 1500}
With no changes to the existing UI or data format, it ends up looking like this (the mis-rendering is probably a bug):
And here's it in the up-and-coming UI by @rogeralsing
|
@adriancole until better times, we're going with JSON encoding (see PR), unless the only field is |
|
|
here's the story in the new zipkin UI On Tue, Sep 20, 2016 at 10:36 AM, Adrian Cole adrian.f.cole@gmail.com wrote:
|
|
@basvanbeek thoughts? |
|
Sorry for the late response, I will take a look this weekend. |
|
@basvanbeek admittedly, the real problem here is that Go doesn't have a good story on semver... that said, I would like to merge opentracing/opentracing-go#108 and it will break this package unless something like this PR 25 is merged. Are you ok with me merging the OT-Go change? If not, any guidance about when you can get to the remaining details in this PR? Let me know, thanks... |
|
superseded by #26 ... closing! |
Handle OpenTracing key-value logging, finished work in PR #25
…_hdr_report adds a summary latency report and a latency CSV file output option
This builds against opentracing/opentracing-go#108. @basvanbeek this is not "Done" done but should be a good start. I'll add a few questions inline.