-
-
Notifications
You must be signed in to change notification settings - Fork 631
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
Add checks if values of flags zipkin-trace-id and zipkin-parent-id are valid #7242
Changes from 2 commits
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 | ||||
---|---|---|---|---|---|---|
|
@@ -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.') | ||||||
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.
Suggested change
|
||||||
register('--zipkin-sample-rate', advanced=True, default=100.0, | ||||||
help='Rate at which to sample Zipkin traces. Value 0.0 - 100.0.') | ||||||
|
||||||
|
@@ -112,6 +111,18 @@ 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)): | ||||||
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. You can actually use 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 didn't know about this one! https://docs.python.org/2.7/library/string.html This looks better than using a regex! |
||||||
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)): | ||||||
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. Maybe pull out a helper function called |
||||||
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) | ||||||
|
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.