Skip to content

test : Add test to confirm Context expected behavior #2724

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

Merged
merged 3 commits into from
Feb 28, 2025

Conversation

cijothomas
Copy link
Member

@cijothomas cijothomas commented Feb 27, 2025

To assist reviewing https://github.com/open-telemetry/opentelemetry-rust/pull/2378/files I wrote some tests, which left me wondering with a unresolved question.

Context MUST be immutable, so adding value to context (with_value), should result in a new Context, leaving original untouched. A test for this is added and works as expected.
Then I tried this with Spans - Created an empty context, and added a Span to the Context. When I add another random value to that Context, it is cloning the random value, but the Span is stored via Arc, so clone is just pointing to the original span. Updates made to span from the new context updates the original span.
Not sure if this is the intended behavior, opening to discuss.

Edit: The expectation is Context itself is immutable, but the Span inside is mutable. Tests modified to assert this.

@cijothomas cijothomas requested a review from a team as a code owner February 27, 2025 03:04
@scottgerring
Copy link
Contributor

I used this as an excuse to have a read of the spec 😄 My read is that the Span can and is designed to mutate internally without the surrounding Context caring.

A Context is immutable: Set Value operation

The API MUST return a new Context containing the new value.

Also, A SpanContext is immutable, but this doesn't matter much for the present discussion:

SpanContexts are immutable.

Whereas, Spans are very mutable - "Set Attributes", "Add Events", etc.

None of these operations nor the section Context Interaction mention anything about span mutation forcing a corresponding update/replace on the context. This would be in principal quite strange 2-way coupling - the Span having to tell the thing that happens to contain it that it had changed.

@cijothomas
Copy link
Member Author

@bantonsson @scottgerring Thanks for clearing up the confusion I had. I modified the tests now to test the expected behavior!

Copy link

codecov bot commented Feb 27, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.4%. Comparing base (fb74565) to head (aef1e3d).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #2724     +/-   ##
=======================================
+ Coverage   79.3%   79.4%   +0.1%     
=======================================
  Files        123     123             
  Lines      22682   22737     +55     
=======================================
+ Hits       17992   18059     +67     
+ Misses      4690    4678     -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cijothomas cijothomas changed the title test:Add test to confirm Context expected behavior test :Add test to confirm Context expected behavior Feb 27, 2025
@cijothomas cijothomas changed the title test :Add test to confirm Context expected behavior test : Add test to confirm Context expected behavior Feb 27, 2025
@cijothomas cijothomas merged commit 91ae096 into open-telemetry:main Feb 28, 2025
22 of 23 checks passed
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.

5 participants