Add support for vulture sending long running traces #951
Conversation
|
This will require #944 |
zalegrala
force-pushed
the
vultureLongWrite
branch
3 times, most recently
from
75a3c00
to
3c21291
Compare
|
This is working pretty good locally. There are two conditions that could result in false positives that I know about currently. The first is that the number of bytes on a trace may exceed the allowed limit of Tempo, which is hard to know unless we query the API to know the size. But perhaps some work on that API should precede that so we can work with json. In lieu of working on the API and reading the configured value from Tempo, I've reduced the chance of extending a trace to include future amendments to a given trace. This seems like maybe a reasonable approach, since for each extension of a trace, the chance of extending it yet again continues to diminish. The second is that when we go to read what we expect is the result, we are effectively counting the number of writes that we expect has taken place. There is a margin of error, where if the timing worked out just perfect, the I'll leave it running locally for a while and see where we're at. |
zalegrala
force-pushed
the
vultureLongWrite
branch
2 times, most recently
from
11ff31c
to
2688012
Compare
zalegrala
requested review from
annanay25,
dgzlopes,
joe-elliott,
kvrhdn,
mapno and
mdisibio
as code owners
cmd/tempo-vulture/main.go
Outdated
| zap.Int64("seed", info.timestamp.Unix()), | ||
| ) | ||
|
|
||
| if maybe(info.r) { |
There was a problem hiding this comment.
i'd like to look at a better way to do this. perhaps we could generate a "total batches" int from the random number generator and store it in traceinfo. then we could decrement that value each time we emitted a batch?
There was a problem hiding this comment.
Good call, I think that cleans up the readability a bit. I've made the update.
cmd/tempo-vulture/main.go
Outdated
| // If the last write has happened very recently, we'll wait a bit to make | ||
| // sure the write has taken place, and then add the batches that would have | ||
| // just been written. | ||
| if time.Since(lastWrite) < (1 * time.Second) { |
There was a problem hiding this comment.
this is pretty janky.
from a seed we should be able to determine when a trace was completed and either query or not query the trace if we feel it is not complete.
putting sleeps here is going to weirdly impact the timings of other loops.
There was a problem hiding this comment.
Alright, I've washed off some of that jank. No more sleeps to construct the expected trace, and we only search/query when we expect that all writes have taken place.
zalegrala
force-pushed
the
vultureLongWrite
branch
from
8388921
to
3d29093
Compare
cmd/tempo-vulture/main.go
Outdated
| // that we get the expected number of batches on a trace. A value larger | ||
| // than 25 here results in vulture writing traces that exceed the maximum | ||
| // trace size. | ||
| batchHighWaterMark int64 = 25 |
There was a problem hiding this comment.
rename:
maxBatchesPerWrite
maxLongWritesPerTrace
3d29093
to
fc19df5
Compare
zalegrala
force-pushed
the
vultureLongWrite
branch
from
fc19df5
to
81eb68f
Compare
What this PR does:
Here we implement the ability for the vulture to send long-running traces, which will enable the vulture verify additional infrastructure components required to resolve the trace.
Which issue(s) this PR fixes:
Fixes #791
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]