Skip to content

fix(sdk/metric): Fix data race with initialization of NewPeriodicReader - #9028

Open
CTrando wants to merge 11 commits into
open-telemetry:mainfrom
CTrando:fix-periodic-reader-inst-race
Open

CTrando wants to merge 11 commits into
open-telemetry:mainfrom
CTrando:fix-periodic-reader-inst-race

Conversation

@CTrando

@CTrando CTrando commented Sep 24, 2026

Copy link
Copy Markdown

Fixes #8769

Just have to move the initialization of a field before the goroutine starts up. Test works by creating a bunch of workers with a small timeout on the goroutine to replicate the data race.

You have to run the test with the -race flag if you want to reproduce the data race test failure.

@linux-foundation-easycla

linux-foundation-easycla Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: CTrando / name: Cameron Trando (81fadb8)

@codecov

codecov Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.1%. Comparing base (c80f83a) to head (9af0045).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #9028     +/-   ##
=======================================
- Coverage   89.1%   89.1%   -0.1%     
=======================================
  Files        338     338             
  Lines      22274   22274             
=======================================
- Hits       19859   19857      -2     
- Misses      2415    2417      +2     
Files with missing lines Coverage Δ
sdk/metric/periodic_reader.go 87.4% <100.0%> (-0.6%) ⬇️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread sdk/metric/periodic_reader_test.go Outdated

@ps-mir ps-mir left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The fix looks ok. But test is not a reliable regression guard, specially when cpu contention is high.

Is there a way observ.NewInstrumentation can be held long enough. TestPeriodicReaderInstrumentationError could be of some use.

Comment thread sdk/metric/periodic_reader_test.go Outdated
@CTrando
CTrando requested a review from ps-mir September 28, 2026 17:03

@ps-mir ps-mir left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Much better, This test is deterministic in producing the Reader non registration and fails 60/60 without fix.

A efficiency nit for reducing time.After.

Comment thread sdk/metric/periodic_reader_test.go Outdated
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.

Data race when initializing PeriodicReader with short interval

3 participants