From af08ed555c9a411a39b84e699c52a6cf5373ed78 Mon Sep 17 00:00:00 2001 From: Ekaterina Tyurina Date: Wed, 13 Feb 2019 15:39:34 +0000 Subject: [PATCH 1/4] Add check that values of flags zipkin-trace-id and zipkin-parent-id are valid --- src/python/pants/reporting/reporting.py | 8 ++++ src/python/pants/reporting/zipkin_reporter.py | 3 +- .../pants_test/reporting/test_reporting.py | 40 +++++++++++++++++++ 3 files changed, 50 insertions(+), 1 deletion(-) diff --git a/src/python/pants/reporting/reporting.py b/src/python/pants/reporting/reporting.py index 76f07e84e87..5ece49c313e 100644 --- a/src/python/pants/reporting/reporting.py +++ b/src/python/pants/reporting/reporting.py @@ -112,6 +112,14 @@ def initialize(self, run_tracker, all_options, start_time=None): raise ValueError( "Flags zipkin-trace-id and zipkin-parent-id must both either be set or not set." ) + if trace_id and len(trace_id) != 16 and len(trace_id) != 32: + raise ValueError( + "Value of the flag zipkin-trace-id must be a 16-character or 32-character hex string." + ) + if parent_id and len(parent_id) != 16: + raise ValueError( + "Value of the flag zipkin-parent-id must be a 16-character hex string." + ) if zipkin_endpoint is not None: zipkin_reporter_settings = ZipkinReporter.Settings(log_level=Report.INFO) diff --git a/src/python/pants/reporting/zipkin_reporter.py b/src/python/pants/reporting/zipkin_reporter.py index 68dd252df43..1787640e0cb 100644 --- a/src/python/pants/reporting/zipkin_reporter.py +++ b/src/python/pants/reporting/zipkin_reporter.py @@ -95,7 +95,8 @@ def start_workunit(self, workunit): span_name=workunit.name, transport_handler=self.handler, encoding=Encoding.V1_THRIFT, - zipkin_attrs=zipkin_attrs + zipkin_attrs=zipkin_attrs, + use_128bit_trace_id=len(zipkin_attrs.trace_id) == 32 ) else: span = zipkin_span( diff --git a/tests/python/pants_test/reporting/test_reporting.py b/tests/python/pants_test/reporting/test_reporting.py index 3df1144481c..d8e61375639 100644 --- a/tests/python/pants_test/reporting/test_reporting.py +++ b/tests/python/pants_test/reporting/test_reporting.py @@ -94,3 +94,43 @@ def test_raise_if_no_parent_id_and_zipkin_endpoint_set(self): "Flags zipkin-trace-id and zipkin-parent-id must both either be set or not set." in str(result.exception) ) + + def test_raise_if_parent_id_is_of_wrong_format(self): + parent_id = 'ff' + options = {'reporting': { + 'zipkin_trace_id': self.trace_id, + 'zipkin_parent_id': parent_id, + 'zipkin_endpoint': self.zipkin_endpoint + }} + context = self.context(for_subsystems=[RunTracker, Reporting], options=options) + + run_tracker = RunTracker.global_instance() + reporting = Reporting.global_instance() + + with self.assertRaises(ValueError) as result: + reporting.initialize(run_tracker, context.options) + + self.assertTrue( + "Value of the flag zipkin-parent-id must be a 16-character hex string." + in str(result.exception) + ) + + def test_raise_if_trace_id_is_of_wrong_format(self): + trace_id = 'aa' + options = {'reporting': { + 'zipkin_trace_id': trace_id, + 'zipkin_parent_id': self.parent_id, + 'zipkin_endpoint': self.zipkin_endpoint + }} + context = self.context(for_subsystems=[RunTracker, Reporting], options=options) + + run_tracker = RunTracker.global_instance() + reporting = Reporting.global_instance() + + with self.assertRaises(ValueError) as result: + reporting.initialize(run_tracker, context.options) + + self.assertTrue( + "Value of the flag zipkin-trace-id must be a 16-character or 32-character hex string." + in str(result.exception) + ) From 80355cbaf2906c60c26dbea2c94ddfcb4f7751ce Mon Sep 17 00:00:00 2001 From: Ekaterina Tyurina Date: Thu, 14 Feb 2019 12:13:19 +0000 Subject: [PATCH 2/4] Address comments --- src/python/pants/reporting/reporting.py | 27 +++++----- src/python/pants/reporting/zipkin_reporter.py | 3 +- .../pants_test/reporting/test_reporting.py | 52 +++++++++++++++++-- 3 files changed, 64 insertions(+), 18 deletions(-) diff --git a/src/python/pants/reporting/reporting.py b/src/python/pants/reporting/reporting.py index 5ece49c313e..95f3fbf01e9 100644 --- a/src/python/pants/reporting/reporting.py +++ b/src/python/pants/reporting/reporting.py @@ -51,15 +51,14 @@ def register_options(cls, register): help='The full HTTP URL of a zipkin server to which traces should be posted. ' 'No traces will be made if this is not set.') register('--zipkin-trace-id', advanced=True, default=None, - help='The overall 64 or 128-bit ID of the trace. ' - 'Set if Pants trace should be a part of larger trace ' - 'for systems that invoke Pants. If zipkin-trace-id ' - 'and zipkin-parent-id are not set, a trace_id value is randomly generated for a ' - 'Zipkin trace') + help='The overall 64 or 128-bit ID of the trace (the format is 16-character or ' + '32-character hex string). Set if Pants trace should be a part of larger trace ' + 'for systems that invoke Pants. If flags zipkin-trace-id and zipkin-parent-id ' + 'are not set, a trace_id value is randomly generated for a Zipkin trace.') register('--zipkin-parent-id', advanced=True, default=None, - help='The 64-bit ID for a parent span that invokes Pants. ' - 'zipkin-trace-id and zipkin-parent-id must both either be set or not set ' - 'when run Pants command') + help='The 64-bit ID for a parent span that invokes Pants (the format is 16-character ' + 'hex string). Flags zipkin-trace-id and zipkin-parent-id must both either be set ' + 'or not set when run Pants command.') register('--zipkin-sample-rate', advanced=True, default=100.0, help='Rate at which to sample Zipkin traces. Value 0.0 - 100.0.') @@ -112,13 +111,17 @@ def initialize(self, run_tracker, all_options, start_time=None): raise ValueError( "Flags zipkin-trace-id and zipkin-parent-id must both either be set or not set." ) - if trace_id and len(trace_id) != 16 and len(trace_id) != 32: + if trace_id and (len(trace_id) != 16 and len(trace_id) != 32 or \ + any(ch not in set('0123456789abcdefABCDEF') for ch in trace_id)): raise ValueError( - "Value of the flag zipkin-trace-id must be a 16-character or 32-character hex string." + "Value of the flag zipkin-trace-id must be a 16-character or 32-character hex string. " + + "Got {}.".format(trace_id) ) - if parent_id and len(parent_id) != 16: + if parent_id and (len(parent_id) != 16 or \ + any(ch not in set('0123456789abcdefABCDEF') for ch in parent_id)): raise ValueError( - "Value of the flag zipkin-parent-id must be a 16-character hex string." + "Value of the flag zipkin-parent-id must be a 16-character hex string. " + + "Got {}.".format(parent_id) ) if zipkin_endpoint is not None: diff --git a/src/python/pants/reporting/zipkin_reporter.py b/src/python/pants/reporting/zipkin_reporter.py index 1787640e0cb..68dd252df43 100644 --- a/src/python/pants/reporting/zipkin_reporter.py +++ b/src/python/pants/reporting/zipkin_reporter.py @@ -95,8 +95,7 @@ def start_workunit(self, workunit): span_name=workunit.name, transport_handler=self.handler, encoding=Encoding.V1_THRIFT, - zipkin_attrs=zipkin_attrs, - use_128bit_trace_id=len(zipkin_attrs.trace_id) == 32 + zipkin_attrs=zipkin_attrs ) else: span = zipkin_span( diff --git a/tests/python/pants_test/reporting/test_reporting.py b/tests/python/pants_test/reporting/test_reporting.py index d8e61375639..b221ed0a97a 100644 --- a/tests/python/pants_test/reporting/test_reporting.py +++ b/tests/python/pants_test/reporting/test_reporting.py @@ -95,7 +95,7 @@ def test_raise_if_no_parent_id_and_zipkin_endpoint_set(self): in str(result.exception) ) - def test_raise_if_parent_id_is_of_wrong_format(self): + def test_raise_if_parent_id_is_of_wrong_len_format(self): parent_id = 'ff' options = {'reporting': { 'zipkin_trace_id': self.trace_id, @@ -111,11 +111,12 @@ def test_raise_if_parent_id_is_of_wrong_format(self): reporting.initialize(run_tracker, context.options) self.assertTrue( - "Value of the flag zipkin-parent-id must be a 16-character hex string." + "Value of the flag zipkin-parent-id must be a 16-character hex string. " + + "Got {}.".format(parent_id) in str(result.exception) ) - def test_raise_if_trace_id_is_of_wrong_format(self): + def test_raise_if_trace_id_is_of_wrong_len_format(self): trace_id = 'aa' options = {'reporting': { 'zipkin_trace_id': trace_id, @@ -131,6 +132,49 @@ def test_raise_if_trace_id_is_of_wrong_format(self): reporting.initialize(run_tracker, context.options) self.assertTrue( - "Value of the flag zipkin-trace-id must be a 16-character or 32-character hex string." + "Value of the flag zipkin-trace-id must be a 16-character or 32-character hex string. " + + "Got {}.".format(trace_id) + in str(result.exception) + ) + + def test_raise_if_parent_id_is_of_wrong_ch_format(self): + parent_id = 'gggggggggggggggg' + options = {'reporting': { + 'zipkin_trace_id': self.trace_id, + 'zipkin_parent_id': parent_id, + 'zipkin_endpoint': self.zipkin_endpoint + }} + context = self.context(for_subsystems=[RunTracker, Reporting], options=options) + + run_tracker = RunTracker.global_instance() + reporting = Reporting.global_instance() + + with self.assertRaises(ValueError) as result: + reporting.initialize(run_tracker, context.options) + + self.assertTrue( + "Value of the flag zipkin-parent-id must be a 16-character hex string. " + + "Got {}.".format(parent_id) + in str(result.exception) + ) + + def test_raise_if_trace_id_is_of_wrong_ch_format(self): + trace_id = 'gggggggggggggggg' + options = {'reporting': { + 'zipkin_trace_id': trace_id, + 'zipkin_parent_id': self.parent_id, + 'zipkin_endpoint': self.zipkin_endpoint + }} + context = self.context(for_subsystems=[RunTracker, Reporting], options=options) + + run_tracker = RunTracker.global_instance() + reporting = Reporting.global_instance() + + with self.assertRaises(ValueError) as result: + reporting.initialize(run_tracker, context.options) + + self.assertTrue( + "Value of the flag zipkin-trace-id must be a 16-character or 32-character hex string. " + + "Got {}.".format(trace_id) in str(result.exception) ) From b4d659cb6f3014aa3cce0cb76d54df64a91b56e1 Mon Sep 17 00:00:00 2001 From: Ekaterina Tyurina Date: Thu, 14 Feb 2019 20:18:37 +0000 Subject: [PATCH 3/4] Update format check of traceId and parentId --- src/python/pants/reporting/reporting.py | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/python/pants/reporting/reporting.py b/src/python/pants/reporting/reporting.py index 95f3fbf01e9..28744b91bd4 100644 --- a/src/python/pants/reporting/reporting.py +++ b/src/python/pants/reporting/reporting.py @@ -52,13 +52,14 @@ def register_options(cls, register): 'No traces will be made if this is not set.') register('--zipkin-trace-id', advanced=True, default=None, help='The overall 64 or 128-bit ID of the trace (the format is 16-character or ' - '32-character hex string). Set if Pants trace should be a part of larger trace ' - 'for systems that invoke Pants. If flags zipkin-trace-id and zipkin-parent-id ' - 'are not set, a trace_id value is randomly generated for a Zipkin trace.') + '32-character hex string). Set if the Pants trace should be a part of a larger ' + 'trace for systems that invoke Pants. If flags zipkin-trace-id and ' + 'zipkin-parent-id are not set, a trace_id value is randomly generated ' + 'for a Zipkin trace.') register('--zipkin-parent-id', advanced=True, default=None, help='The 64-bit ID for a parent span that invokes Pants (the format is 16-character ' 'hex string). Flags zipkin-trace-id and zipkin-parent-id must both either be set ' - 'or not set when run Pants command.') + 'or not set when running a Pants command.') register('--zipkin-sample-rate', advanced=True, default=100.0, help='Rate at which to sample Zipkin traces. Value 0.0 - 100.0.') @@ -111,14 +112,12 @@ def initialize(self, run_tracker, all_options, start_time=None): raise ValueError( "Flags zipkin-trace-id and zipkin-parent-id must both either be set or not set." ) - if trace_id and (len(trace_id) != 16 and len(trace_id) != 32 or \ - any(ch not in set('0123456789abcdefABCDEF') for ch in trace_id)): + if trace_id and (len(trace_id) != 16 and len(trace_id) != 32 or not is_hex_string(trace_id)): raise ValueError( "Value of the flag zipkin-trace-id must be a 16-character or 32-character hex string. " + "Got {}.".format(trace_id) ) - if parent_id and (len(parent_id) != 16 or \ - any(ch not in set('0123456789abcdefABCDEF') for ch in parent_id)): + if parent_id and (len(parent_id) != 16 or not is_hex_string(parent_id)): raise ValueError( "Value of the flag zipkin-parent-id must be a 16-character hex string. " + "Got {}.".format(parent_id) @@ -206,3 +205,10 @@ def update_reporting(self, global_options, is_quiet, run_tracker): invalidation_report.set_filename(outfile) return invalidation_report + +def is_hex_string(id_value): + return all(is_hex_ch(ch) for ch in id_value) + +def is_hex_ch(ch): + num = ord(ch) + return ord('0') <= num <= ord('9') or ord('a') <= num <= ord('f') or ord('A') <= num <= ord('F') From 035c11a748f2e00abd9e4cdf42b6384329915445 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Thu, 14 Feb 2019 15:04:38 -0800 Subject: [PATCH 4/4] Fix lint. --- src/python/pants/reporting/reporting.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/python/pants/reporting/reporting.py b/src/python/pants/reporting/reporting.py index 28744b91bd4..245f4eef4cc 100644 --- a/src/python/pants/reporting/reporting.py +++ b/src/python/pants/reporting/reporting.py @@ -206,9 +206,11 @@ def update_reporting(self, global_options, is_quiet, run_tracker): return invalidation_report + def is_hex_string(id_value): return all(is_hex_ch(ch) for ch in id_value) + def is_hex_ch(ch): num = ord(ch) return ord('0') <= num <= ord('9') or ord('a') <= num <= ord('f') or ord('A') <= num <= ord('F')