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

[TextFormatPropagator] fix parent trace_option data type to string #714

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hkwi
Copy link

@hkwi hkwi commented Jul 4, 2019

DEFAULT_TRACE_OPTIONS is string, so mixing bool here has broken situation as following:

>>> from opencensus.trace.propagation.text_format import TextFormatPropagator
>>> propagator = TextFormatPropagator()
>>> data = {}
>>> span_context = propagator.from_carrier(data)
>>> span_context.trace_options.set_enabled(True)
>>> print(propagator.to_carrier(span_context, data))
{'opencensus-trace-traceid': 'ad39a21155167cc115a13e5f4cfb6cd4', 'opencensus-trace-traceoptions': '1'}
>>> span_context = propagator.from_carrier(data)
>>> span_context.trace_options.set_enabled(True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/mnt/c/Users/kawai/Documents/kwi-opencensus/opencensus/trace/trace_options.py", line 78, in set_enabled
    self.enabled = self.get_enabled()
  File "/mnt/c/Users/kawai/Documents/kwi-opencensus/opencensus/trace/trace_options.py", line 65, in get_enabled
    enabled = bool(int(self.trace_options_byte) & _ENABLED_BITMASK)
ValueError: invalid literal for int() with base 10: 'Tru1'
>>> print(propagator.to_carrier(span_context, data))
{'opencensus-trace-traceid': 'ad39a21155167cc115a13e5f4cfb6cd4', 'opencensus-trace-traceoptions': 'Tru1'}

DEFAULT_TRACE_OPTIONS is string, so mixing bool here has broken situation as following:

```
>>> from opencensus.trace.propagation.text_format import TextFormatPropagator
>>> propagator = TextFormatPropagator()
>>> data = {}
>>> span_context = propagator.from_carrier(data)
>>> span_context.trace_options.set_enabled(True)
>>> print(propagator.to_carrier(span_context, data))
{'opencensus-trace-traceid': 'ad39a21155167cc115a13e5f4cfb6cd4', 'opencensus-trace-traceoptions': '1'}
>>> span_context = propagator.from_carrier(data)
>>> span_context.trace_options.set_enabled(True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/mnt/c/Users/kawai/Documents/kwi-opencensus/opencensus/trace/trace_options.py", line 78, in set_enabled
    self.enabled = self.get_enabled()
  File "/mnt/c/Users/kawai/Documents/kwi-opencensus/opencensus/trace/trace_options.py", line 65, in get_enabled
    enabled = bool(int(self.trace_options_byte) & _ENABLED_BITMASK)
ValueError: invalid literal for int() with base 10: 'Tru1'
>>> print(propagator.to_carrier(span_context, data))
{'opencensus-trace-traceid': 'ad39a21155167cc115a13e5f4cfb6cd4', 'opencensus-trace-traceoptions': 'Tru1'}
```
@hkwi hkwi requested review from c24t, reyang, songy23 and a team as code owners July 4, 2019 01:22
@reyang
Copy link
Contributor

reyang commented Jul 4, 2019

@hkwi thanks for the fix. It would be great to add a test case.

@@ -49,7 +49,7 @@ def from_carrier(self, carrier):
if key == _SPAN_ID_KEY:
span_id = carrier[key]
if key == _TRACE_OPTIONS_KEY:
trace_options = bool(carrier[key])
trace_options = carrier[key]
Copy link
Contributor

Choose a reason for hiding this comment

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

Not directly related to this PR. It seems we don't have any validation here. @c24t for awareness, we need to have a better implementation going forward.

@c24t
Copy link
Member

c24t commented Jul 12, 2019

LGTM with a test case that fails on master, passes with this change.

@reyang
Copy link
Contributor

reyang commented Jul 16, 2019

@hkwi would you add a test case and also rebase to the latest master? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants