-
Notifications
You must be signed in to change notification settings - Fork 155
Added Python3 support #43
Changes from 4 commits
771bcb0
1373dcd
05bcb4a
cfe390c
2612c73
af5b703
d1740b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
from builtins import str | ||
import json | ||
import logging | ||
|
||
|
@@ -93,7 +94,7 @@ def traced_service_object_to_json(obj): | |
|
||
|
||
def set_traced_service_object_values(obj, values, downstream_func): | ||
for k in values.iterkeys(): | ||
for k in values.keys(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume similar issue here, unnecessary allocation in 2.7 |
||
if hasattr(obj, k): | ||
if k == 'downstream': | ||
if values[k] is not None: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
from builtins import object | ||
import logging | ||
|
||
import tornado.web | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,8 +19,12 @@ | |
# THE SOFTWARE. | ||
|
||
from __future__ import absolute_import | ||
from future import standard_library | ||
standard_library.install_aliases() | ||
from past.builtins import basestring | ||
from builtins import object | ||
|
||
import urllib | ||
import urllib.request, urllib.parse, urllib.error | ||
|
||
from opentracing import ( | ||
InvalidCarrierException, | ||
|
@@ -64,37 +68,37 @@ def inject(self, span_context, carrier): | |
parent_id=span_context.parent_id, flags=span_context.flags) | ||
baggage = span_context.baggage | ||
if baggage: | ||
for key, value in baggage.iteritems(): | ||
for key, value in baggage.items(): | ||
if self.url_encoding: | ||
encoded_value = urllib.quote(value) | ||
encoded_value = urllib.parse.quote(value) | ||
else: | ||
encoded_value = value | ||
carrier['%s%s' % (self.baggage_prefix, key)] = encoded_value | ||
|
||
def extract(self, carrier): | ||
if not hasattr(carrier, 'iteritems'): | ||
if not isinstance(carrier, dict): | ||
raise InvalidCarrierException('carrier not a collection') | ||
trace_id, span_id, parent_id, flags = None, None, None, None | ||
baggage = None | ||
debug_id = None | ||
for key, value in carrier.iteritems(): | ||
for key, value in carrier.items(): | ||
uc_key = key.lower() | ||
if uc_key == self.trace_id_header: | ||
if self.url_encoding: | ||
value = urllib.unquote(value) | ||
value = urllib.parse.unquote(value) | ||
trace_id, span_id, parent_id, flags = \ | ||
span_context_from_string(value) | ||
elif uc_key.startswith(self.baggage_prefix): | ||
if self.url_encoding: | ||
value = urllib.unquote(value) | ||
value = urllib.parse.unquote(value) | ||
attr_key = key[self.prefix_length:] | ||
if baggage is None: | ||
baggage = {attr_key.lower(): value} | ||
else: | ||
baggage[attr_key.lower()] = value | ||
elif uc_key == self.debug_id_header: | ||
if self.url_encoding: | ||
value = urllib.unquote(value) | ||
value = urllib.parse.unquote(value) | ||
debug_id = value | ||
if not trace_id and baggage: | ||
raise SpanContextCorruptedException('baggage without trace ctx') | ||
|
@@ -137,7 +141,7 @@ def span_context_to_string(trace_id, span_id, parent_id, flags): | |
:param parent_id: | ||
:param flags: | ||
""" | ||
parent_id = parent_id or 0L | ||
parent_id = parent_id or 0 | ||
return '{:x}:{:x}:{:x}:{:x}'.format(trace_id, span_id, parent_id, flags) | ||
|
||
|
||
|
@@ -162,9 +166,9 @@ def span_context_from_string(value): | |
raise SpanContextCorruptedException( | ||
'malformed trace context "%s"' % value) | ||
try: | ||
trace_id = long(parts[0], 16) | ||
span_id = long(parts[1], 16) | ||
parent_id = long(parts[2], 16) | ||
trace_id = int(parts[0], 16) | ||
span_id = int(parts[1], 16) | ||
parent_id = int(parts[2], 16) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as I understand Here is an output from my 2.7 console:
This also means that replacing
|
||
flags = int(parts[3], 16) | ||
if trace_id < 1 or span_id < 1 or parent_id < 0 or flags < 0: | ||
raise SpanContextCorruptedException( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,10 +33,10 @@ | |
DEFAULT_FLUSH_INTERVAL = 1 | ||
|
||
# Name of the HTTP header used to encode trace ID | ||
TRACE_ID_HEADER = b'uber-trace-id' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I remember we explicitly ran into an issue with this string being Unicode in some instrumentation of urllib2. Why can we not keep this as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe in Python 2.7 string and bytes types are essentially equivalent:
They are different in Python 3 though:
So, with this string being explicitly marked as bytes, I had the following error when running tests in Python3:
Which makes sense, because
So, weighing the options of changing every affected test and changing just single line Talking about urllib2 issue, I'm not sure what could have caused it given that bytes and strings are same in Py2 – maybe you have an example? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found these comments in the commit when we changed headers to
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And this was the error stack trace
The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess I should've written a test for that (facepalm) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yurishkuro Seems like I can't reproduce the error, so a test case would be beneficial... I'm also not sure how switching Here is what I've tried to do (with some variations):
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So here's the scenario in Python 2.7, which I believe reflects what was happening:
The code in httplib.py looks like this def _send_output(self, message_body=None):
. . .
if isinstance(message_body, str):
msg += message_body Now, I don't know why
And then later when the body with invalid seq is appended, Python tries to parse it as Unicode and blows up. For this reason I had to declare the headers as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Trying to fix this in #109 |
||
TRACE_ID_HEADER = 'uber-trace-id' | ||
|
||
# Prefix for HTTP headers used to record baggage items | ||
BAGGAGE_HEADER_PREFIX = b'uberctx-' | ||
BAGGAGE_HEADER_PREFIX = 'uberctx-' | ||
|
||
# The name of HTTP header or a TextMap carrier key which, if found in the | ||
# carrier, forces the trace to be sampled as "debug" trace. The value of the | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,9 @@ | |
# THE SOFTWARE. | ||
|
||
from __future__ import absolute_import | ||
from __future__ import division | ||
from builtins import object | ||
from past.utils import old_div | ||
import logging | ||
import random | ||
import json | ||
|
@@ -45,7 +48,7 @@ | |
SAMPLER_TYPE_TAG_KEY = 'sampler.type' | ||
SAMPLER_PARAM_TAG_KEY = 'sampler.param' | ||
DEFAULT_SAMPLING_PROBABILITY = 0.001 | ||
DEFAULT_LOWER_BOUND = 1.0 / (10.0 * 60.0) # sample once every 10 minutes | ||
DEFAULT_LOWER_BOUND = old_div(1.0, (10.0 * 60.0)) # sample once every 10 minutes | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the reason for this change? Doesn't Python3 work with I'd like to have comments in the code for non-trivial decisions. |
||
DEFAULT_MAX_OPERATIONS = 2000 | ||
|
||
STRATEGIES_STR = 'perOperationStrategies' | ||
|
@@ -235,7 +238,7 @@ def update(self, lower_bound, rate): | |
|
||
def __str__(self): | ||
return 'GuaranteedThroughputProbabilisticSampler(%s, %s, %s)' \ | ||
% (self.operation, self.rate, self.lower_bound) | ||
% (self.operation, self.rate, round(float(self.lower_bound), 14)) | ||
|
||
|
||
class AdaptiveSampler(Sampler): | ||
|
@@ -306,12 +309,12 @@ def update(self, strategies): | |
ProbabilisticSampler(self.default_sampling_probability) | ||
|
||
def close(self): | ||
for _, sampler in self.samplers.iteritems(): | ||
for _, sampler in self.samplers.items(): | ||
sampler.close() | ||
|
||
def __str__(self): | ||
return 'AdaptiveSampler(%s, %s, %s)' \ | ||
% (self.default_sampling_probability, self.lower_bound, | ||
% (self.default_sampling_probability, round(float(self.lower_bound), 14), | ||
self.max_operations) | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
from past.builtins import basestring | ||
# Copyright (c) 2016 Uber Technologies, Inc. | ||
# | ||
# Permission is hereby granted, free of charge, to any person obtaining a copy | ||
|
@@ -20,6 +21,7 @@ | |
|
||
import socket | ||
import struct | ||
import sys | ||
|
||
import jaeger_client.thrift_gen.zipkincore.ZipkinCollector as zipkin_collector | ||
import jaeger_client.thrift_gen.sampling.SamplingManager as sampling_manager | ||
|
@@ -30,8 +32,12 @@ | |
|
||
_max_signed_port = (1 << 15) - 1 | ||
_max_unsigned_port = (1 << 16) | ||
_max_signed_id = (1L << 63) - 1 | ||
_max_unsigned_id = (1L << 64) | ||
_max_signed_id = (1 << 63) - 1 | ||
_max_unsigned_id = (1 << 64) | ||
|
||
|
||
def str_to_binary(value): | ||
return value if sys.version_info[0] == 2 else value.encode('utf-8') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if this is checking the python version, I would rather use |
||
|
||
|
||
def ipv4_to_int(ipv4): | ||
|
@@ -61,6 +67,12 @@ def port_to_int(port): | |
def id_to_int(big_id): | ||
# zipkincore.thrift defines ID fields as i64, which is signed, | ||
# therefore we convert large IDs (> 2^63) to negative longs | ||
|
||
# In Python 2, expression None > 1 is legal and has a value of False | ||
# In Python 3, this expression is illegal - so we need to have an additional check | ||
if big_id is None: | ||
return None | ||
|
||
if big_id > _max_signed_id: | ||
big_id -= _max_unsigned_id | ||
return big_id | ||
|
@@ -78,8 +90,9 @@ def make_endpoint(ipv4, port, service_name): | |
def make_string_tag(key, value): | ||
if len(value) > 256: | ||
value = value[:256] | ||
|
||
return zipkin_collector.BinaryAnnotation( | ||
key, value, zipkin_collector.AnnotationType.STRING) | ||
key, str_to_binary(value), zipkin_collector.AnnotationType.STRING) | ||
|
||
|
||
def make_peer_address_tag(key, host): | ||
|
@@ -90,7 +103,7 @@ def make_peer_address_tag(key, host): | |
:param host: | ||
""" | ||
return zipkin_collector.BinaryAnnotation( | ||
key, '0x01', zipkin_collector.AnnotationType.BOOL, host) | ||
key, str_to_binary('0x01'), zipkin_collector.AnnotationType.BOOL, host) | ||
|
||
|
||
def make_local_component_tag(component_name, endpoint): | ||
|
@@ -100,7 +113,7 @@ def make_local_component_tag(component_name, endpoint): | |
:param endpoint: | ||
""" | ||
return zipkin_collector.BinaryAnnotation( | ||
key=LOCAL_COMPONENT, value=component_name, | ||
key=LOCAL_COMPONENT, value=str_to_binary(component_name), | ||
annotation_type=zipkin_collector.AnnotationType.STRING, | ||
host=endpoint) | ||
|
||
|
@@ -117,7 +130,7 @@ def timestamp_micros(ts): | |
:param ts: | ||
:return: | ||
""" | ||
return long(ts * 1000000) | ||
return int(ts * 1000000) | ||
|
||
|
||
def make_zipkin_spans(spans): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not an equivalent change.
itemitems()
in 2.7 returns an iterator, whileitems()
creates a full copy.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree. I was coming from an assumption that there will be not so many tags per span and spans will be sampled (i.e. 1 span per 100 or 1000 operation runs) so the performance was not a huge concern from my point of view.
To keep performance on the same level, something like this helper function should probably work better:
Well, it's already a part of the
six
package – I'll go ahead and change that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok addressed this in 2612c73