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

set_status: use update_element since select_replace doesn't use index #384

Merged
merged 1 commit into from
Apr 27, 2022

Conversation

tsloughter
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Apr 26, 2022

Codecov Report

Merging #384 (6cf7b36) into main (f9996de) will increase coverage by 0.00%.
The diff coverage is 83.33%.

@@           Coverage Diff           @@
##             main     #384   +/-   ##
=======================================
  Coverage   73.29%   73.29%           
=======================================
  Files          53       53           
  Lines        1640     1644    +4     
=======================================
+ Hits         1202     1205    +3     
- Misses        438      439    +1     
Flag Coverage Δ
api 68.70% <ø> (ø)
elixir 17.54% <ø> (ø)
erlang 74.73% <83.33%> (+<0.01%) ⬆️
exporter 74.41% <ø> (ø)
sdk 78.08% <83.33%> (-0.02%) ⬇️
zipkin 53.84% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
apps/opentelemetry/src/otel_span_ets.erl 72.09% <83.33%> (+0.29%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f9996de...6cf7b36. Read the comment docs.

Copy link
Member

@ferd ferd left a comment

Choose a reason for hiding this comment

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

I figure there's a risk where you can run into races with this, but given the current format of the writes and the possibility of getting a false result if a delete has occured, this appears to be safe in context given the current operations.

@ferd
Copy link
Member

ferd commented Apr 26, 2022

There could be a small race where the call to make a span status as ok after it was error interleaves oddly with scheduling and fails (by setting ok before error). I'm not sure how concerned you are with that.

@tsloughter
Copy link
Member Author

Yea, technically the possibility of a race is part of every operation in this module. But a span should only be updated by a single process. This is not currently enforced since it would require somehow tracking the "owner" (can't just be the process that started it since it could be created to be passed to a newly spawned process), but since the majority of usage will be as the current span stored in the process dictionary it isn't something too be concerned about I think.

@tsloughter tsloughter merged commit 8b355ee into open-telemetry:main Apr 27, 2022
@tsloughter tsloughter deleted the rm-select-replace branch April 27, 2022 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants