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.

Sign up for GitHub

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

Jump to bottom

Protect access to Span implementation #1188

Merged
merged 3 commits into from
Oct 6, 2020
Merged

Protect access to Span implementation #1188

merged 3 commits into from
Oct 6, 2020

Conversation

ahlaw
Copy link
Contributor

@ahlaw ahlaw commented Oct 1, 2020 •
edited
Loading

Description

Span now throws a TypeError in the constructor, and all instances of Span must now be instantiated by _Span constructor. The was implemented by overriding the __new__ constructor in the Span class.

Fixes #1000

Type of change

Please delete options that are not relevant.

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

  • Unit test to verify an exception will be thrown when the constructor is called without the additional argument
  • Tox suite to check no existing functionality has been broken

Checklist:

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

@ahlaw ahlaw requested a review from a team as a code owner October 1, 2020 00:20
owais
owais approved these changes Oct 3, 2020
View reviewed changes
Copy link
Contributor

@owais owais left a comment

Choose a reason for hiding this comment

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

LGTM

opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py Outdated Show resolved Hide resolved
aabmass
aabmass approved these changes Oct 3, 2020
View reviewed changes
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

Nice I like this approach!

codeboten
codeboten suggested changes Oct 5, 2020
View reviewed changes
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.

One suggested change based on @owais's comment. Otherwise this change looks good

@ahlaw ahlaw requested a review from codeboten October 5, 2020 19:39
@codeboten codeboten merged commit 6fac795 into open-telemetry:master Oct 6, 2020
@ahlaw ahlaw deleted the 1000-private-sdk-span branch October 7, 2020 18:29
@shovnik shovnik mentioned this pull request Dec 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@owais owais owais approved these changes

@codeboten codeboten codeboten approved these changes

@aabmass aabmass aabmass approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implementations MUST NOT allow callers to create Spans directly
4 participants
@ahlaw @owais @codeboten @aabmass