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

Make Instances of SpanContext Immutable #1134

Merged

Conversation

JasonXZLiu
Copy link
Member

@JasonXZLiu JasonXZLiu commented Sep 17, 2020

Description

This PR makes instances of SpanContext Immutable to match the OpenTelemetry specifications.

Fixes #: #1002

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Unit tests have been added to test the following behavior:

  • instantiating a SpanContext returns an object with the correct values set
  • trying to change an attribute throws TypeError
  • after trying to change an attribute, the object maintains its original values

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

Note: documentation did not need updating as SpanContext is already assumed to be Immutable (from the specifications).

@JasonXZLiu JasonXZLiu requested a review from a team as a code owner September 17, 2020 17:24
@JasonXZLiu JasonXZLiu force-pushed the 1002-make-spancontext-immutable branch from c7f2675 to e6de321 Compare September 17, 2020 18:33
@lzchen
Copy link
Contributor

lzchen commented Sep 21, 2020

Please add a CHANGELOG entry as this may break users who have already been trying to modify spancontext.

@JasonXZLiu JasonXZLiu force-pushed the 1002-make-spancontext-immutable branch from 02d7b3f to b9f0d20 Compare September 22, 2020 01:31
opentelemetry-api/src/opentelemetry/trace/span.py Outdated Show resolved Hide resolved

is_valid = trace_id != INVALID_TRACE_ID and span_id != INVALID_SPAN_ID

return tuple.__new__(

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.

Copy link
Member Author

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.

Copy link
Member Author

@JasonXZLiu JasonXZLiu Sep 22, 2020

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:

opentelemetry-api/src/opentelemetry/trace/span.py:200: error: "SpanContext" has no attribute "trace_id"
opentelemetry-api/src/opentelemetry/trace/span.py:200: error: Expression has type "Any"
opentelemetry-api/src/opentelemetry/trace/span.py:201: error: "SpanContext" has no attribute "span_id"
opentelemetry-api/src/opentelemetry/trace/span.py:201: error: Expression has type "Any"
opentelemetry-api/src/opentelemetry/trace/span.py:202: error: "SpanContext" has no attribute "trace_state"
opentelemetry-api/src/opentelemetry/trace/span.py:202: error: Expression has type "Any"
opentelemetry-api/src/opentelemetry/trace/span.py:203: error: "SpanContext" has no attribute "is_remote"
opentelemetry-api/src/opentelemetry/trace/span.py:203: error: Expression has type "Any"

I haven't found a good way around this yet (as only Python 3.6+ supports annotating variables with types).

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 :)

Copy link
Contributor

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.

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.

Copy link
Member Author

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.

@JasonXZLiu JasonXZLiu force-pushed the 1002-make-spancontext-immutable branch 2 times, most recently from 4106fa7 to 920e75e Compare September 22, 2020 05:49
.pylintrc Outdated
@@ -70,7 +70,8 @@ disable=missing-docstring,
wrong-import-order, # Leave this up to isort
bad-continuation, # Leave this up to black
line-too-long, # Leave this up to black
exec-used
exec-used,
unsubscriptable-object
Copy link
Member Author

@JasonXZLiu JasonXZLiu Sep 22, 2020

Choose a reason for hiding this comment

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

I was getting unsubscriptable-object errors through the linter for the self[0] lines. I tried using getattr instead, but that gave me mypy errors for the attribute types. Couldn't find a better way to get around this change.

Choose a reason for hiding this comment

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

Do you know what true negatives we end up with because of these changes? Ideally we don't reduce the robustness of the tooling significantly by making this small change. This relates to a comment I made on another PR I think in that it's important to make sure the SDK as a whole is optimized for, and not individual suggestions. Actually I've recently found the spec might have too much detail that makes it hard to reconcile with language-specific design like here - open-telemetry/opentelemetry-specification#969. If immutable types aren't idiomatic in Python and cause our tooling to break, we need to consider whether it's actually worth it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, it's true that immutable types aren't idiomatic in Python. From @lzchen's comment below, it seems like it might be possible to disable pylint specifically for just the self[0] lines (as above) which I'll try first. However, it does seem like pylint does have a lot of false positive issues with the unsubscriptable-object error (like here).

mypy-relaxed.ini Outdated Show resolved Hide resolved
@codeboten
Copy link
Contributor

@anuraaga & @owais if you're ok with this PR, please mark as approved

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

@JasonXZLiu if you resolve the conflict, I can merge today

@JasonXZLiu JasonXZLiu force-pushed the 1002-make-spancontext-immutable branch from 4ee86cb to e20c5c4 Compare October 5, 2020 16:19
@codeboten codeboten merged commit 568641f into open-telemetry:master Oct 5, 2020
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
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.

None yet

6 participants