tracing: refactor the trace recorder #4756
Conversation
602bb11 to
3cf5e76
Compare
util/tracing/detect/recorder.go
Outdated
| type TraceRecorder struct { | ||
| sdktrace.SpanExporter | ||
| // mu is a channel that acts as a mutex for this struct. |
There was a problem hiding this comment.
mu means mutex type. Maybe you are looking for https://pkg.go.dev/golang.org/x/sync/semaphore#Weighted.Acquire ?
There was a problem hiding this comment.
Yea this will work fine.
util/tracing/detect/recorder.go
Outdated
|
|
||
| spanCtx := trace.SpanContextFromContext(ctx) | ||
| if !spanCtx.IsValid() { | ||
| return emptyTraceSpans |
There was a problem hiding this comment.
Why this stub instead of just returning nil like above?
There was a problem hiding this comment.
The original idea was because then this wouldn't trigger a "no trace recorder" error. I looked through the original path and it's probably just easier if we return nil everywhere than the stub empty function so I've removed this.
util/tracing/detect/recorder.go
Outdated
|
|
||
| if err := r.lock(ctx); err != nil { | ||
| return emptyTraceSpans |
There was a problem hiding this comment.
If we can exit out because of a context error, we should return that error.
util/tracing/detect/recorder.go
Outdated
| } | ||
|
|
||
| func (r *TraceRecorder) Shutdown(ctx context.Context) error { | ||
| if r.SpanExporter == nil { | ||
| // Initiate the shutdown of the gc loop. | ||
| r.shutdown(context.Canceled) |
util/tracing/detect/recorder.go
Outdated
| } | ||
|
|
||
| func (r *TraceRecorder) Shutdown(ctx context.Context) error { | ||
| if r.SpanExporter == nil { | ||
| // Initiate the shutdown of the gc loop. | ||
| r.shutdown(context.Canceled) |
There was a problem hiding this comment.
Deja vu github? If there's an area where I've missed adding errors.WithStack please tell me, but github seems to be showing me the same comment twice?
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case now := <-ticker.C: |
There was a problem hiding this comment.
There is a difference in here from old code that under load this ticker will not start to slow down.
There was a problem hiding this comment.
I'm not sure what you mean. I believe ticker automatically drops ticks or slows down for slow receivers: https://pkg.go.dev/time#Ticker
Looking at the old code, it does appear that the old one would have 1 minute as the initial timer and then 50 seconds for every subsequent interval. I'm not sure if that was intentional or not.
util/tracing/detect/recorder.go
Outdated
| mu chan struct{} | ||
|
|
||
| // shutdown function for the gc. | ||
| shutdown func(err error) |
0c466e1 to
a0eb0d1
Compare
util/tracing/detect/recorder.go
Outdated
| // lock is a binary semaphore for this struct. | ||
| // This is used instead of sync.Mutex because it allows | ||
| // for context cancellation to work properly. | ||
| lock *semaphore.Weighted |
There was a problem hiding this comment.
nit: I would use a sem or sema name in here just so that there isn't any confusion about the Locker interface patterns.
util/tracing/detect/recorder.go
Outdated
| r.mu.Lock() | ||
| defer r.mu.Unlock() | ||
| func (r *TraceRecorder) Record(ctx context.Context) (func() []tracetest.SpanStub, error) { | ||
| // Just a simplification for usage. |
There was a problem hiding this comment.
"Just a simplification for usage."
That looks more like a review-help comment than code comment. As reader of code does not know the history.
"If the trace recorder was never initialized, return no function."
This sentence can be left in. Or it can be turned into function comment `// Record starts to record all the spans from the trace associated with the current span context and returns a function that returns all such spans when called. If the trace recorder was never initialized, return no function."
a0eb0d1 to
22033ba
Compare
The trace recorder is now a separate entity and does not wrap another span exporter. It is added as another span processor to the underlying tracer provider so the two can be disconnected from each other and this removes the need to use `detect.Exporter()` to find the TraceRecorder along with the need to invoke flush manually. The trace recorder is wrapped with a simple span processor instead of the batch one. That makes the flushing irrelevant for this purpose. The trace recorder itself is also modified to avoid leaking goroutines and respecting context cancellations. This is part of a general refactor of the detect package to separate the buildkit-specific functionality from the external exporter detection. Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
22033ba to
0e1cf1c
Compare
The trace recorder is now a separate entity and does not wrap another span exporter. It is added as another span processor to the underlying tracer provider so the two can be disconnected from each other and this removes the need to use
detect.Exporter()to find the TraceRecorder along with the need to invoke flush manually.The trace recorder is wrapped with a simple span processor instead of the batch one. That makes the flushing irrelevant for this purpose.
The trace recorder itself is also modified to avoid leaking goroutines and respecting context cancellations.
This is part of a general refactor of the detect package to separate the buildkit-specific functionality from the external exporter detection.