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 OpenTracing propagator #302

Merged
merged 23 commits into from
Feb 16, 2021
Merged

Add OpenTracing propagator #302

merged 23 commits into from
Feb 16, 2021

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Jan 28, 2021

Description

Adds propagator for OpenTracing.

Fixes #294

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

  • Test A

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

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

@ocelotl ocelotl requested review from a team, owais and aabmass and removed request for a team January 28, 2021 06:10
Base automatically changed from master to main January 29, 2021 19:28
@ocelotl ocelotl force-pushed the issue_294 branch 3 times, most recently from 2998d39 to 1c64838 Compare February 4, 2021 22:20
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.

Looks good, just a few minor suggestions

CHANGELOG.md Outdated Show resolved Hide resolved
propagator/opentelemetry-propagator-ot-trace/README.rst Outdated Show resolved Hide resolved
propagator/opentelemetry-propagator-ot-trace/README.rst Outdated Show resolved Hide resolved
propagator/opentelemetry-propagator-ot-trace/setup.cfg Outdated Show resolved Hide resolved
propagator/opentelemetry-propagator-ot-trace/setup.cfg Outdated Show resolved Hide resolved
Comment on lines 41 to 44
_valid_header_name = re_compile(r"^[\w_^`!#$%&'*+.|~]+$")
_valid_header_value = re_compile(r"^[\t\x20-\x7e\x80-\xff]+$")
_valid_traceid = re_compile(r"[0-9a-f]{32}|[0-9a-f]{16}")
_valid_spanid = re_compile(r"[0-9a-f]{16}")
Copy link
Member

Choose a reason for hiding this comment

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

nit CONSTANT_CASE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, what is the issue here?

Copy link
Member

Choose a reason for hiding this comment

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

just a nit, if these are constants consider _valid_header_name -> _VALID_HEADER_NAME

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PEP8 says that is how constants should be cased. AFAIK, constants are only True, False, None, strings, and numerical values.

Copy link
Member

Choose a reason for hiding this comment

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

Some compiled regexes in constant case here as well https://github.com/open-telemetry/opentelemetry-python/blob/0fe1f476a05bfa055a868d8433488a785bb65cdc/opentelemetry-api/src/opentelemetry/util/tracestate.py#L49-L52

I believe the purpose of this casing is to indicate the variable is a constant and shouldn't be modified, which is the intention here. int, float, bool, string, etc. aren't special in python, they are just classes right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some compiled regexes in constant case here as well https://github.com/open-telemetry/opentelemetry-python/blob/0fe1f476a05bfa055a868d8433488a785bb65cdc/opentelemetry-api/src/opentelemetry/util/tracestate.py#L49-L52

Yeah..., we should also fix those 😅

I believe the purpose of this casing is to indicate the variable is a constant and shouldn't be modified, which is the intention here. int, float, bool, string, etc. aren't special in python, they are just classes right?

Hmnot really. int, float, bool, None, strings are special, because they are actually constants and they are created only once. This can be checked like this:

ocelotl@harrison:~$ python
Python 3.8.3 (default, Jun 29 2020, 18:03:36) 
[GCC 7.5.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> a = 1
>>> b = 1
>>> id(a)
93987911005952
>>> id(b)
93987911005952
>>> a is b
True
>>> a = "a"
>>> b = "a"
>>> id(a)
140646739870512
>>> id(b)
140646739870512
>>> a is b
True
>>> a = None
>>> b = None
>>> id(a)
93987910826592
>>> id(b)
93987910826592
>>> a is b
True
>>> 

As it can be seen here, any variable assigned to the same int will "point" to the same object, ints are true constants in that sense, they will not change during the program execution.

The same does not happen for other objects:

ocelotl@harrison:~$ python
Python 3.8.3 (default, Jun 29 2020, 18:03:36) 
[GCC 7.5.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> class Test:
...     pass
... 
>>> a = Test()
>>> b = Test()
>>> id(a)
139882366030992
>>> id(b)
139882366749904
>>> a is b
False
>>> 

Now, let's try with compiled regular expressions:

ocelotl@harrison:~$ python
fPython 3.8.3 (default, Jun 29 2020, 18:03:36) 
[GCC 7.5.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from re import compile as re_compile
>>> a = re_compile(r"abc")
>>> b = re_compile(r"abc")
>>> id(a)
140532296161216
>>> id(b)
140532296161216
>>> a is b
True
>>> 

Now, this looks like compiled regular expressions are indeed constants the same as ints are, but that is just an illusion caused by an implementation detail. The re module uses an artificial cache as an optimization to keep there already compiled regular expressions. That cache has a limit so at some moment even compiled regular expressions may be compiled again for the exact same pattern and flags. Now, I am getting just pretty pedantic here with the definition of a constant, 😅 but I wanted to explain why compiled regular expressions are not constants in the most strict sense.

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.

LGTM

@ocelotl ocelotl requested a review from lzchen February 16, 2021 23:00
Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

Question about version number.

@ocelotl ocelotl requested a review from lzchen February 16, 2021 23:41
@lzchen lzchen merged commit 269e0f4 into open-telemetry:main Feb 16, 2021
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.

Add OpenTracing propagator
4 participants