-
Notifications
You must be signed in to change notification settings - Fork 155
Added Python3 support #43
Changes from all 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 object | ||
import logging | ||
|
||
import tornado.web | ||
|
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,9 +19,13 @@ | |
# 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 | ||
import six | ||
|
||
from threading import Lock | ||
from tornado.ioloop import PeriodicCallback | ||
|
@@ -45,7 +49,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 +239,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 +310,12 @@ def update(self, strategies): | |
ProbabilisticSampler(self.default_sampling_probability) | ||
|
||
def close(self): | ||
for _, sampler in self.samplers.iteritems(): | ||
for _, sampler in six.iteritems(self.samplers): | ||
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.
why is
int
equivalent tolong
? isn't it dependent on the architecture?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.
As far as I understand
int
andlong
types were pretty much unified as of Python 2.4: https://www.python.org/dev/peps/pep-0237/. In Python 3 any further distinction was erased: there is nolong()
function and noL
postfix for long integer types.Here is an output from my 2.7 console:
This also means that replacing
_max_unsigned_id = (1L << 64)
with_max_unsigned_id = (1 << 64)
somewhere else in this PR is also probably safe: