Conversation
LeoCavaille
changed the title
|
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.
clutchski
changed the title
| // 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. |
… EVP flagevaluation contract - #1 (blocker): route globalCap overflow to ultra-degraded instead of dropping; count total preserved at every tier - #3: add TestSaturationCountPreservation (total counts == add() calls past both globalCap and perFlagCap) - #2: correct collision-safe wording to low-collision/not collision-proof; document count-preserving collision behavior - #4: rotate flag keys + distinct context field names in benchmark so real cardinality is exercised
…rsal Address oleksii PR #4886 review #4: the worker path previously ran flattenContext (flatten.go) then pruneContext as two separate steps, each allocating its own map. Merge them into a single flattenAndPruneContext used by aggregate(): flatten nested objects, then apply the deterministic sort-before-cut 256-field / 256-char prune in one pass. Output is byte-for-byte identical to the prior two-step pipeline (same surviving keys, same 256/256 limits, same deterministic cut). When the flattened context already fits the limits — the common case — the flattened map is returned directly, eliding the second pruned-output map the old pipeline always allocated. flattenContext is left unchanged because exposure_hook.go still calls it; only the EVP aggregation path is merged. pruneContext is retained as the reference behavior for tests. Added TestFlattenAndPruneContextEquivalence asserting the merged pass equals flattenContext+pruneContext across nested, oversized, and >256-field inputs, with preserved determinism.
3 participants
span.Finishand co.Span.Print()and a debug logging flag so we can see what's flowing while developing / debugging.