-
Notifications
You must be signed in to change notification settings - Fork 651
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
Make Instances of SpanContext Immutable #1134
Merged
codeboten
merged 7 commits into
open-telemetry:master
from
open-o11y:1002-make-spancontext-immutable
Oct 5, 2020
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
36aa772
Override setattr and delattr for SpanContext + add tests
756be5f
Fix typing errors
73eba70
Fix linting issues on span.py
765e689
Reordered tuple fields
5caccab
Removed casting of fields
35a0d78
Added logging and fixed mypy error
e20c5c4
Remove pylint + mypy config changes
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
58 changes: 58 additions & 0 deletions
58
opentelemetry-api/tests/trace/test_immutablespancontext.py
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,58 @@ | ||
# Copyright The OpenTelemetry Authors | ||
# | ||
# Licensed under the Apache License, Version 2.0 (the "License"); | ||
# you may not use this file except in compliance with the License. | ||
# You may obtain a copy of the License at | ||
# | ||
# http://www.apache.org/licenses/LICENSE-2.0 | ||
# | ||
# Unless required by applicable law or agreed to in writing, software | ||
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
import unittest | ||
|
||
from opentelemetry import trace | ||
from opentelemetry.trace import TraceFlags, TraceState | ||
|
||
|
||
class TestImmutableSpanContext(unittest.TestCase): | ||
def test_ctor(self): | ||
context = trace.SpanContext( | ||
1, | ||
1, | ||
is_remote=False, | ||
trace_flags=trace.DEFAULT_TRACE_OPTIONS, | ||
trace_state=trace.DEFAULT_TRACE_STATE, | ||
) | ||
|
||
self.assertEqual(context.trace_id, 1) | ||
self.assertEqual(context.span_id, 1) | ||
self.assertEqual(context.is_remote, False) | ||
self.assertEqual(context.trace_flags, trace.DEFAULT_TRACE_OPTIONS) | ||
self.assertEqual(context.trace_state, trace.DEFAULT_TRACE_STATE) | ||
|
||
def test_attempt_change_attributes(self): | ||
context = trace.SpanContext( | ||
1, | ||
2, | ||
is_remote=False, | ||
trace_flags=trace.DEFAULT_TRACE_OPTIONS, | ||
trace_state=trace.DEFAULT_TRACE_STATE, | ||
) | ||
|
||
# attempt to change the attribute values | ||
context.trace_id = 2 # type: ignore | ||
context.span_id = 3 # type: ignore | ||
context.is_remote = True # type: ignore | ||
context.trace_flags = TraceFlags(3) # type: ignore | ||
context.trace_state = TraceState([("test", "test")]) # type: ignore | ||
|
||
# check if attributes changed | ||
self.assertEqual(context.trace_id, 1) | ||
self.assertEqual(context.span_id, 2) | ||
self.assertEqual(context.is_remote, False) | ||
self.assertEqual(context.trace_flags, trace.DEFAULT_TRACE_OPTIONS) | ||
self.assertEqual(context.trace_state, trace.DEFAULT_TRACE_STATE) |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we have to override setattr anyways is there an advantage of using tuple vs normal fields set with super.setattr? Latter might be a bit simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, if I understand correctly, I think the advantage of inheriting from a tuple is that it doesn't allow something like
object.__setattr__(context, "trace_id", 2)
(while not inheriting does). I'm not sure if that's something we want to avoid or if we should just keep it simple.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anuraaga Hmm, it actually seems a lot cleaner and I dont think the case I mentioned above is something that needs to be avoided. But, because
mypy
doesn't actually run the code, it's not able to determine that SpanContext actually has the attributes, so I get the following errors:I haven't found a good way around this yet (as only Python 3.6+ supports annotating variables with types).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see - in that case no worries :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to make it impossible to use
__setattr__
? This should be immutable to prevent users from the mistake of using SpanContext to additional information around. I think making that impossible to do through the public API serves that purpose and self-documents the class. I don't think we need to make it literally impossible to modify the object. Given this is Python, I think anyone included to do it will find a way. We should just make sure we inform the person that they shouldn't do it. If they want to go out of their way and still modify, I think we should probably allow that.I'm personally fine with using tuple for this but if it is causing too much trouble with tooling and needs a lot of other machinery, perhaps we could use a simpler way that protect modifications through public API but doesn't actually try to make it impossible for users to modify the object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@owais @anuraaga Hmm, I think I solved the tooling issues now (the ignores should be inline). There seems to be a false-positive for
pylint
in determining if SpanContext was instantiated as a tuple (here: https://github.com/open-telemetry/opentelemetry-python/pull/1134/files#diff-36ec0e77c55d7f4a4caef71eda0b3191R187). The issue for testing if we could change the attributes was also fixed inline (here: https://github.com/open-telemetry/opentelemetry-python/pull/1134/files#diff-0924a59286313e337f50e00112a27df8R46-R51).I'm not sure if it's worth it to go through with the change as it seems like Python doesn't support immutability very well, but all the tooling changes that I made globally were removed.