Empty queued spans when ForceFlush called #2335

pkwarren · 2021-10-28T19:50:22Z

Update the implementation of ForceFlush() to first ensure that all spans
which are queued are added to the batch before calling export spans.
Create a small ReadOnlySpan implementation which can be used as a marker
that ForceFlush has been invoked and used to notify when all spans are
ready to be exported.

Fixes #2080.

Update the implementation of ForceFlush() to first ensure that all spans which are queued are added to the batch before calling export spans. Create a small ReadOnlySpan implementation which can be used as a marker that ForceFlush has been invoked and used to notify when all spans are ready to be exported. Fixes #2080.

linux-foundation-easycla · 2021-10-28T19:50:24Z

The committers are authorized under a signed CLA.

✅ Philip K. Warren (b2dd3c9, 0e621b8, c7e4c51, 4eb4f2b, 0180f37, cb23081, bf24f4c)

seanschade

I like the simplicity of this solution.

Aneurysm9

Please add a CHANGELOG entry describing this change and sign the CLA.

MrAlias

The ForceFlush method should not shut down the processor.

CHANGELOG.md

sdk/trace/batch_span_processor.go

sdk/trace/batch_span_processor_test.go

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

MrAlias

Please sign the CLA so we can accept these changes.

MrAlias · 2021-10-29T17:21:30Z

sdk/trace/batch_span_processor.go

 // ForceFlush exports all ended spans that have not yet been exported.
 func (bsp *batchSpanProcessor) ForceFlush(ctx context.Context) error {
 	var err error
 	if bsp.e != nil {
+		flushCh := make(chan struct{})
+		select {
+		case bsp.queue <- forceFlushSpan{flushed: flushCh}:


Maybe use the enque method here instead to handle the shutdown case.

Though this blocks on a full queue ... It might make sense to copy some of the logic from that method to here to handle the case where shutdown has already occurred or is concurrently happening.

There is still more that needs to be changed here.

pkwarren · 2021-10-29T17:26:36Z

Please sign the CLA so we can accept these changes.

Will do this ASAP - waiting on legal sign off.

pkwarren · 2021-10-29T18:04:18Z

sdk/trace/batch_span_processor.go

+}
+
+func (f forceFlushSpan) SpanContext() trace.SpanContext {
+	return trace.NewSpanContext(trace.SpanContextConfig{TraceFlags: trace.FlagsSampled})


Using enqueue requires that the span being queued is marked as sampled.

pkwarren · 2021-10-29T18:04:58Z

sdk/trace/batch_span_processor.go

 // ForceFlush exports all ended spans that have not yet been exported.
 func (bsp *batchSpanProcessor) ForceFlush(ctx context.Context) error {
 	var err error
 	if bsp.e != nil {
+		flushCh := make(chan struct{})
+		bsp.enqueueBlockOnQueueFull(ctx, forceFlushSpan{flushed: flushCh}, true)


Pass true as the last argument here since we want to block for this span to be queued.

pkwarren · 2021-10-29T18:05:41Z

sdk/trace/batch_span_processor.go

@@ -296,6 +320,10 @@ func (bsp *batchSpanProcessor) drainQueue() {
 }

 func (bsp *batchSpanProcessor) enqueue(sd ReadOnlySpan) {
+	bsp.enqueueBlockOnQueueFull(context.TODO(), sd, bsp.o.BlockOnQueueFull)


In order to work in the ForceFlush method, it was needed to pass a context (to allow cancellation).

Update the handling of the force flush span so we only wait on the channel if we were able to enqueue the span to the queue.

pkwarren · 2021-11-03T13:44:52Z

Sorry for delay on CLA - that is now approved. Does this need a CI build to be approved now?

codecov · 2021-11-03T13:51:17Z

Codecov Report

Merging #2335 (486120a) into main (7ce58f3) will increase coverage by 0.1%.
The diff coverage is 82.6%.

@@           Coverage Diff           @@
##            main   #2335     +/-   ##
=======================================
+ Coverage   73.6%   73.8%   +0.1%     
=======================================
  Files        175     175             
  Lines      12419   12436     +17     
=======================================
+ Hits        9144    9179     +35     
+ Misses      3041    3020     -21     
- Partials     234     237      +3

Impacted Files	Coverage Δ
sdk/trace/batch_span_processor.go	`83.7% <82.6%> (-0.7%)`	⬇️
sdk/trace/provider.go	`88.4% <0.0%> (+4.8%)`	⬆️
sdk/trace/snapshot.go	`96.9% <0.0%> (+42.4%)`	⬆️

pkwarren · 2021-11-03T17:32:39Z

There is a test TestNew_collectorConnectionDiesThenReconnects failing with this PR when running the tests with the race detector in exporters/otlp/otlptrace/otlptracegrpc. I'm able to reproduce the same failure locally on the main branch with this command:

$ cd exporters/otlp/otlptrace/otlptracegrpc
$ for in in `seq 1 100`; do go test ./... -run '^\QTestNew_collectorConnectionDiesThenReconnects\E$' -count 1 -race -v; done

Is this a known flaky test? I also saw it referenced in #2285.

Aneurysm9 · 2021-11-04T20:23:48Z

I think there have been a couple attempts to address that flake test, but it has thus far eluded correction.

pkwarren requested review from Aneurysm9, dashpole, evantorrie, jmacd, MadVikingGod, MrAlias, paivagustavo, pellared and XSAM as code owners October 28, 2021 19:50

seanschade approved these changes Oct 28, 2021

View reviewed changes

Aneurysm9 added the blocked:CLA Waiting on CLA to be signed before progress can be made label Oct 29, 2021

Aneurysm9 approved these changes Oct 29, 2021

View reviewed changes

Add a changelog entry.

0e621b8

MrAlias requested changes Oct 29, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

sdk/trace/batch_span_processor.go Outdated Show resolved Hide resolved

Aneurysm9 reviewed Oct 29, 2021

View reviewed changes

sdk/trace/batch_span_processor_test.go Outdated Show resolved Hide resolved

pkwarren and others added 3 commits October 29, 2021 11:59

Update CHANGELOG.md

c7e4c51

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

Update sdk/trace/batch_span_processor.go

4eb4f2b

Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>

Improve test case to verify multiple flushes.

0180f37

MrAlias previously approved these changes Oct 29, 2021

View reviewed changes

MrAlias reviewed Oct 29, 2021

View reviewed changes

Refactor code to use enqueue.

cb23081

pkwarren commented Oct 29, 2021

View reviewed changes

seanschade approved these changes Oct 29, 2021

View reviewed changes

Aneurysm9 approved these changes Oct 29, 2021

View reviewed changes

Be more defensive on waiting for queue.

bf24f4c

Update the handling of the force flush span so we only wait on the channel if we were able to enqueue the span to the queue.

Fix linter.

c03a61a

Aneurysm9 removed the blocked:CLA Waiting on CLA to be signed before progress can be made label Nov 4, 2021

Merge branch 'main' into issue-2080

486120a

jmacd approved these changes Nov 5, 2021

View reviewed changes

Aneurysm9 merged commit 6d2aeb0 into open-telemetry:main Nov 5, 2021

Aneurysm9 mentioned this pull request Nov 5, 2021

Lambda instrumentation enhancements open-telemetry/opentelemetry-go-contrib#1390

Closed

pkwarren deleted the issue-2080 branch November 5, 2021 18:20

This was referenced Nov 9, 2021

Go lambda instrumentation enhancements open-telemetry/opentelemetry-go-contrib#1413

Merged

Release prep v1.2.0/v0.25.0 #2377

Merged

pellared added this to the untracked milestone Nov 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Empty queued spans when ForceFlush called #2335

Empty queued spans when ForceFlush called #2335

pkwarren commented Oct 28, 2021

linux-foundation-easycla bot commented Oct 28, 2021 •

edited

Loading

seanschade left a comment

Aneurysm9 left a comment

MrAlias left a comment

MrAlias left a comment

MrAlias Oct 29, 2021

MrAlias Oct 29, 2021

pkwarren commented Oct 29, 2021

pkwarren Oct 29, 2021

pkwarren Oct 29, 2021

pkwarren Oct 29, 2021

pkwarren commented Nov 3, 2021

codecov bot commented Nov 3, 2021 •

edited

Loading

pkwarren commented Nov 3, 2021

Aneurysm9 commented Nov 4, 2021

Empty queued spans when ForceFlush called #2335

Empty queued spans when ForceFlush called #2335

Conversation

pkwarren commented Oct 28, 2021

linux-foundation-easycla bot commented Oct 28, 2021 • edited Loading

seanschade left a comment

Choose a reason for hiding this comment

Aneurysm9 left a comment

Choose a reason for hiding this comment

MrAlias left a comment

Choose a reason for hiding this comment

MrAlias left a comment

Choose a reason for hiding this comment

MrAlias Oct 29, 2021

Choose a reason for hiding this comment

MrAlias Oct 29, 2021

Choose a reason for hiding this comment

pkwarren commented Oct 29, 2021

pkwarren Oct 29, 2021

Choose a reason for hiding this comment

pkwarren Oct 29, 2021

Choose a reason for hiding this comment

pkwarren Oct 29, 2021

Choose a reason for hiding this comment

pkwarren commented Nov 3, 2021

codecov bot commented Nov 3, 2021 • edited Loading

Codecov Report

pkwarren commented Nov 3, 2021

Aneurysm9 commented Nov 4, 2021

linux-foundation-easycla bot commented Oct 28, 2021 •

edited

Loading

codecov bot commented Nov 3, 2021 •

edited

Loading