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

Add __contains__ to TraceState #1773

Merged
merged 8 commits into from Apr 16, 2021

Conversation

alertedsnake
Copy link
Contributor

@alertedsnake alertedsnake commented Apr 14, 2021

Description

Adds __contains__ to TraceState - having this prevents accidentally assuming it's present and will work as expected.

This actually fixes a bug reported in the contrib repo: open-telemetry/opentelemetry-python-contrib#262 and possibly others.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • TestTraceContextFormat.test_trace_contains

Does This PR Require a Contrib Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

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

Michael Stella added 3 commits April 14, 2021 14:49
Having this prevents accidentally assuming it's present and will work
as expected.
@alertedsnake alertedsnake marked this pull request as ready for review April 14, 2021 19:36
@alertedsnake alertedsnake requested a review from a team as a code owner April 14, 2021 19:36
@alertedsnake alertedsnake requested review from codeboten and ocelotl and removed request for a team April 14, 2021 19:36
Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

This fixes the issue, approving.

That being said, I think we should instead subclass TraceState from OrderedDict so that we don't have to keep adding these methods by hand. If we want to avoid items being set, we can raise a proper exception in __setitem__.

As @lonewolf3739 suggested, this is the actual fix, it should raise
`KeyError` rather than return `None`.
@alertedsnake alertedsnake changed the title [WIP] Add __contains__ to TraceState Add __contains__ to TraceState Apr 15, 2021
Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

LGTM. Suggested small change.

opentelemetry-api/src/opentelemetry/trace/span.py Outdated Show resolved Hide resolved
@alertedsnake
Copy link
Contributor Author

That broken test seems unrelated, especially since it passed other times, but let me know if there's something I can do.

@ocelotl
Copy link
Contributor

ocelotl commented Apr 15, 2021

That broken test seems unrelated, especially since it passed other times, but let me know if there's something I can do.

Pypy has this behavior sometimes, I am running the tests in this PR again 👍

@lzchen lzchen merged commit 371eaa8 into open-telemetry:main Apr 16, 2021
codeboten pushed a commit to codeboten/opentelemetry-python that referenced this pull request Apr 16, 2021
@alertedsnake alertedsnake deleted the tracestate-contains branch April 17, 2021 16:27
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

4 participants