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

Fixed race condition in OnEnd and added a unit test #3951

Merged
merged 12 commits into from
Apr 14, 2023

Conversation

Kaushal28
Copy link
Contributor

Fixes: #3948

Fixed race condition in OnEnd and added a unit test.

sdk/trace/simple_span_processor_test.go Show resolved Hide resolved
@pellared pellared dismissed their stale review March 30, 2023 13:31

My initial review may be wrong.

Copy link
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.

Thanks for the PR 👍

Can you also update the CHANGELOG.md?

sdk/trace/simple_span_processor_test.go Outdated Show resolved Hide resolved
sdk/trace/simple_span_processor_test.go Outdated Show resolved Hide resolved
sdk/trace/simple_span_processor_test.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 31, 2023

Codecov Report

Merging #3951 (0a2554e) into main (eb2b89f) will not change coverage.
The diff coverage is 100.0%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #3951   +/-   ##
=====================================
  Coverage   82.1%   82.1%           
=====================================
  Files        175     175           
  Lines      12977   12977           
=====================================
  Hits       10655   10655           
  Misses      2102    2102           
  Partials     220     220           
Impacted Files Coverage Δ
sdk/trace/simple_span_processor.go 75.4% <100.0%> (ø)

@Kaushal28
Copy link
Contributor Author

Hey @pellared , I have made all the changes as per your comments and also updated CHANGELOG.md. Please have a look. Thanks!

Copy link
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.

Thanks for your contribution 👍

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Robert Pająk <[email protected]>
sdk/trace/simple_span_processor_test.go Outdated Show resolved Hide resolved
sdk/trace/simple_span_processor_test.go Outdated Show resolved Hide resolved
sdk/trace/simple_span_processor_test.go Show resolved Hide resolved
@Kaushal28
Copy link
Contributor Author

Hey @MrAlias, incorporated your suggestions. Could you please have a look?

@Kaushal28 Kaushal28 requested a review from MrAlias April 3, 2023 10:51
@Kaushal28
Copy link
Contributor Author

@pellared @MrAlias Thanks for pointing it out. I have fixed the test case as suggested. Please have a look. Thanks.

@MrAlias MrAlias merged commit 3738859 into open-telemetry:main Apr 14, 2023
Kaushal28 added a commit to infracloudio/opentelemetry-go that referenced this pull request Apr 21, 2023
)

* Fixed race condition in OnEnd and added a test

* fixed code review comments

* fixed lint

* Update CHANGELOG.md

Co-authored-by: Robert Pająk <[email protected]>

* Update sdk/trace/simple_span_processor_test.go

Co-authored-by: Tyler Yahn <[email protected]>

* Update sdk/trace/simple_span_processor_test.go

Co-authored-by: Tyler Yahn <[email protected]>

* Update sdk/trace/simple_span_processor_test.go

Co-authored-by: Tyler Yahn <[email protected]>

* fixed panic check

---------

Co-authored-by: Robert Pająk <[email protected]>
Co-authored-by: Tyler Yahn <[email protected]>
@MrAlias MrAlias mentioned this pull request Apr 27, 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.

Data race in simple span processor
5 participants