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

sdk/trace: Refine context cancellation in batchSpanProcessor.ForceFlush #4369

Merged
merged 4 commits into from
Jul 27, 2023

Conversation

pellared
Copy link
Member

@pellared pellared commented Jul 26, 2023

Fixes #4368

The issue is that the flush request can be send and processed before <-ctx.Done() is handled. Now we just check if context is not canceled before doing anything else to have more deterministic behavior.

After 0c8d9fa:

$ go test -race -count=100000 -run TestBatchSpanProcessorForceFlushTimeout
--- FAIL: TestBatchSpanProcessorForceFlushTimeout (0.00s)
    batch_span_processor_test.go:571: expected "context deadline exceeded" error, got <nil>
--- FAIL: TestBatchSpanProcessorForceFlushTimeout (0.00s)
    batch_span_processor_test.go:571: expected "context deadline exceeded" error, got <nil>
--- FAIL: TestBatchSpanProcessorForceFlushTimeout (0.01s)
    batch_span_processor_test.go:571: expected "context deadline exceeded" error, got <nil>
--- FAIL: TestBatchSpanProcessorForceFlushTimeout (0.00s)
    batch_span_processor_test.go:571: expected "context deadline exceeded" error, got <nil>
--- FAIL: TestBatchSpanProcessorForceFlushTimeout (0.00s)
    batch_span_processor_test.go:571: expected "context deadline exceeded" error, got <nil>
FAIL
exit status 1
FAIL    go.opentelemetry.io/otel/sdk/trace      72.959s

$ go test -race -count=100000 -run TestBatchSpanProcessorForceFlushCancellation
--- FAIL: TestBatchSpanProcessorForceFlushCancellation (0.00s)
    batch_span_processor_test.go:559: expected "context canceled" error, got <nil>
--- FAIL: TestBatchSpanProcessorForceFlushCancellation (0.00s)
    batch_span_processor_test.go:559: expected "context canceled" error, got <nil>
--- FAIL: TestBatchSpanProcessorForceFlushCancellation (0.00s)
    batch_span_processor_test.go:559: expected "context canceled" error, got <nil>
--- FAIL: TestBatchSpanProcessorForceFlushCancellation (0.00s)
    batch_span_processor_test.go:559: expected "context canceled" error, got <nil>
--- FAIL: TestBatchSpanProcessorForceFlushCancellation (0.00s)
    batch_span_processor_test.go:559: expected "context canceled" error, got <nil>
--- FAIL: TestBatchSpanProcessorForceFlushCancellation (0.00s)
    batch_span_processor_test.go:559: expected "context canceled" error, got <nil>
--- FAIL: TestBatchSpanProcessorForceFlushCancellation (0.00s)
    batch_span_processor_test.go:559: expected "context canceled" error, got <nil>
FAIL
exit status 1
FAIL    go.opentelemetry.io/otel/sdk/trace      55.897s

After 1533304:

$ go test -race -count=100000 -run TestBatchSpanProcessorForceFlushTimeout
PASS
ok      go.opentelemetry.io/otel/sdk/trace      71.522s

$ go test -race -count=100000 -run TestBatchSpanProcessorForceFlushCancellation
PASS
ok      go.opentelemetry.io/otel/sdk/trace      54.733s

@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Merging #4369 (bc6be57) into main (b4264c5) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #4369     +/-   ##
=======================================
- Coverage   83.4%   83.4%   -0.1%     
=======================================
  Files        184     184             
  Lines      14380   14383      +3     
=======================================
- Hits       11999   11998      -1     
- Misses      2153    2157      +4     
  Partials     228     228             
Files Changed Coverage Δ
sdk/trace/batch_span_processor.go 78.4% <100.0%> (-1.5%) ⬇️

@pellared pellared changed the title Refine context cancellation in BatchSpanProcessor.ForceFlush Refine context cancellation in batchSpanProcessor.ForceFlush Jul 26, 2023
@pellared pellared changed the title Refine context cancellation in batchSpanProcessor.ForceFlush sdl/trace: Refine context cancellation in batchSpanProcessor.ForceFlush Jul 26, 2023
@pellared pellared changed the title sdl/trace: Refine context cancellation in batchSpanProcessor.ForceFlush sdk/trace: Refine context cancellation in batchSpanProcessor.ForceFlush Jul 26, 2023
@pellared pellared marked this pull request as ready for review July 26, 2023 16:09
@pellared pellared merged commit 4f0d73c into open-telemetry:main Jul 27, 2023
22 checks passed
@pellared pellared deleted the refine-context-cancelation branch July 27, 2023 08:06
@MrAlias MrAlias added this to the v1.17.0 milestone Aug 3, 2023
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.

Revisit TestBatchSpanProcessorForceFlushTimeout
3 participants