Skip to content

fix(otelzap): write core.ctx only when init#7368

Merged
dmathieu merged 3 commits intoopen-telemetry:mainfrom
okhowang:main
May 28, 2025
Merged

fix(otelzap): write core.ctx only when init#7368
dmathieu merged 3 commits intoopen-telemetry:mainfrom
okhowang:main

Conversation

@okhowang
Copy link
Copy Markdown
Contributor

Fix #7367

@okhowang okhowang requested review from a team and pellared as code owners May 27, 2025 05:11
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented May 27, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: okhowang (8bb35fe)
  • ✅ login: dmathieu / name: Damien Mathieu (efbaed5)
  • ✅ login: pellared / name: Robert Pająk (f4a5cff)

@github-actions github-actions Bot requested a review from khushijain21 May 27, 2025 05:11
Copy link
Copy Markdown
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

Can you please add a changelog entry?

@codecov
Copy link
Copy Markdown

codecov Bot commented May 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.6%. Comparing base (b2aa398) to head (efbaed5).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #7368   +/-   ##
=====================================
  Coverage   81.6%   81.6%           
=====================================
  Files        205     205           
  Lines      18186   18187    +1     
=====================================
+ Hits       14840   14844    +4     
+ Misses      2924    2922    -2     
+ Partials     422     421    -1     
Files with missing lines Coverage Δ
bridges/otelzap/core.go 97.9% <100.0%> (+<0.1%) ⬆️

... and 1 file with indirect coverage changes

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

Copy link
Copy Markdown
Contributor

@khushijain21 khushijain21 left a comment

Choose a reason for hiding this comment

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

Thanks for catching this!

@dmathieu
Copy link
Copy Markdown
Member

Could this be tested?

@okhowang
Copy link
Copy Markdown
Contributor Author

Could this be tested?

it can be found by go test -race, and adding a test to call Write with context.Context field in two goroutine.

@dmathieu
Copy link
Copy Markdown
Member

Sure, but the CI does run go test with -race and doesn't fail on the main branch.

@pellared
Copy link
Copy Markdown
Member

Could this be tested?

it can be found by go test -race, and adding a test to call Write with context.Context field in two goroutine.

Can you add such test?
Notice https://github.com/open-telemetry/opentelemetry-go/blob/main/CONTRIBUTING.md#testing:

Use the term ConcurrentSafe in the test name when it aims to verify the absence of race conditions.

@okhowang
Copy link
Copy Markdown
Contributor Author

test case added

Copy link
Copy Markdown
Member

@dmathieu dmathieu left a comment

Choose a reason for hiding this comment

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

This looks good, we now only need a changelog entry 😸

Comment thread bridges/otelzap/core_test.go Outdated
@okhowang okhowang force-pushed the main branch 3 times, most recently from aa9f78c to 5e91a0b Compare May 27, 2025 10:45
Comment thread CHANGELOG.md Outdated
Comment thread bridges/otelzap/core_test.go
Comment thread CHANGELOG.md Outdated
@dmathieu dmathieu merged commit b06bc8a into open-telemetry:main May 28, 2025
26 of 27 checks passed
@MrAlias MrAlias added this to the v1.37.0 milestone Jun 18, 2025
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 in otelzap's ctx

5 participants