Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modify ForceFlush to abort after timeout/cancellation #1757

Merged
merged 21 commits into from
Apr 1, 2021

Conversation

humivo
Copy link
Member

@humivo humivo commented Mar 30, 2021

Modified the method, BatchSpanProcessor.ForceFlush, to abort after configured timeout or cancellation. Should resolve Issue #1618

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 30, 2021

CLA Signed

The committers are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Mar 30, 2021

Codecov Report

Merging #1757 (93e2ec4) into main (3947cab) will increase coverage by 0.0%.
The diff coverage is 83.3%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #1757   +/-   ##
=====================================
  Coverage   78.6%   78.6%           
=====================================
  Files        133     133           
  Lines       7072    7083   +11     
=====================================
+ Hits        5560    5569    +9     
- Misses      1267    1268    +1     
- Partials     245     246    +1     
Impacted Files Coverage Δ
sdk/trace/batch_span_processor.go 75.0% <83.3%> (+0.6%) ⬆️

CHANGELOG.md Outdated Show resolved Hide resolved
var bp testBatchExporter
bsp := sdktrace.NewBatchSpanProcessor(&bp)

err := bsp.ForceFlush(context.Background())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To test if ForceFlush succeeds it should be verified that if actually flushes any held span batches not just returns a nil error. Can you extend this test to do that?

sdk/trace/batch_span_processor_test.go Outdated Show resolved Hide resolved
@humivo humivo requested a review from MrAlias March 30, 2021 22:02
@humivo humivo marked this pull request as draft March 30, 2021 23:00
@humivo humivo closed this Mar 31, 2021
@humivo humivo reopened this Mar 31, 2021
@Aneurysm9
Copy link
Member

--- FAIL: TestBatchSpanProcessorForceFlushSucceeds (0.00s)
batch_span_processor_test.go:292: number of exported span: got 516, want 517

This has been a recurring issue with attempting to test this span processor. Even though this test has set the queue size to 0 and the processor is configured to block on a full queue, it looks like it's still possible for the last span to be pulled off the queue and the scheduler to switch back to the test and start ForceFlush before processQueue finishes adding the last span to the batch.

I wonder if testing that a non-zero number of spans were exported or that the exported count was within 10 of the expected count might be sufficient here since we simply want to know that ForceFlush shipped spans and the remaining spans can be picked up later. @MrAlias or @open-telemetry/go-approvers any thoughts?

@humivo humivo marked this pull request as ready for review March 31, 2021 17:36
@MrAlias MrAlias merged commit 928e3c3 into open-telemetry:main Apr 1, 2021
@MrAlias MrAlias mentioned this pull request Apr 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BatchSpanProcessor.ForceFlush aborts after configured timeout
3 participants