Conversation
|
The code looks good, but you are failing the test suite because of your test case. |
- adds max size so that we can't take infinite memory of something breaks
downstream
- easier to reason about concurrency
- fixes the following race condition:
if len(t.finishedSpans) > 0 {
t.mu.Lock()
spans := t.finishedSpans
t.finishedSpans = nil
so we can switch on and see info about what you're producing. very good for debugging.
| // buffer list if some fields are missing. The trace agent | ||
| // will discard this span in any case so it's better to prevent | ||
| // more useless work. | ||
| if t.enabled && span.Name != "" && span.Service != "" && span.Resource != "" { |
There was a problem hiding this comment.
i removed this because it means misconfigured spans will silently disappear with no feedback. this is really bad for a user perspective. i'd rather (a) send the data (b) have a validation error appear in the agent logs (c) have the validation code in as few places as possible.
| sb.spans = append(sb.spans, span) | ||
| } else { | ||
| idx := rand.Intn(sb.maxSize) | ||
| sb.spans[idx] = span |
There was a problem hiding this comment.
Is it correct to remove a previously random span if the buffer is full? what happen if we remove a root span and all children lose their parents?
There was a problem hiding this comment.
good question. i was actually thinking that we need to start pushing things as a trace rather than as a list of spans. i will work on this.
There was a problem hiding this comment.
as a trace you mean when the root span is closed and so the children?
| } | ||
| } | ||
|
|
||
| // Strin returns a human readable representation of the span. Not for |
There was a problem hiding this comment.
String instead of Strin
| // Background worker that handles data delivery through the Transport instance. | ||
| // It waits for a flush interval and then it tries to find an available dispatcher | ||
| // if there is something to send. | ||
| // worker flushes |
There was a problem hiding this comment.
is it intentional to remove the comment? maybe it wasn't so helpful but I think that if it's not useful to explain a little bit the overview of the worker function, we can remove it at all
| // buffer list if some fields are missing. The trace agent | ||
| // will discard this span in any case so it's better to prevent | ||
| // more useless work. | ||
| if t.enabled && span.Name != "" && span.Service != "" && span.Resource != "" { |
| } | ||
| } | ||
|
|
||
| if t.enabled && t.transport != nil && 0 < len(spans) { |
There was a problem hiding this comment.
the t.transport may be nil? can it be changed by users? or is it used only to be really sure and robust? (just asking)
About the 0 < len(spans) can't it be len(spans) > 0? I mean for the code style (I know that is stupid but just asking because this should become an open source project).
| } | ||
|
|
||
| // FIXME[matt] on rotation, we could re-use the slices and spans here | ||
| // and avoid re-allocing. |
span.Finishand co.Span.Print()and a debug logging flag so we can see what's flowing while developing / debugging.