Skip to content
Permalink
Browse files

Add checks if values of flags zipkin-trace-id and zipkin-parent-id ar…

…e valid (pantsbuild#7242)

### Problem
When pants are called with flags zipkin-trace-id and zipkin-parent-id an assertion error is raised if the values of the flag are of the wrong format. The error is not informative. 

### Solution
Checks of values of flags zipkin-trace-id and zipkin-parent-id are added with a better error explanation. Users of the pants are asked to use 16-character  or 32-character hex string.
Also, tests are added for these checks.
  • Loading branch information...
cattibrie authored and stuhood committed Feb 15, 2019
1 parent bc0536c commit 594f91fddc03010717d5c8aae7b2889335943987
Showing with 111 additions and 8 deletions.
  1. +27 −8 src/python/pants/reporting/reporting.py
  2. +84 −0 tests/python/pants_test/reporting/test_reporting.py
@@ -51,15 +51,15 @@ 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 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. '
'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 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.')

@@ -112,6 +112,16 @@ 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 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 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)
)

if zipkin_endpoint is not None:
zipkin_reporter_settings = ZipkinReporter.Settings(log_level=Report.INFO)
@@ -195,3 +205,12 @@ 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')
@@ -94,3 +94,87 @@ 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_len_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. "
+ "Got {}.".format(parent_id)
in str(result.exception)
)

def test_raise_if_trace_id_is_of_wrong_len_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. "
+ "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)
)

0 comments on commit 594f91f

Please sign in to comment.
You can’t perform that action at this time.