Skip to content
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

Use UUID from run_id as trace_id if trace_id isn't set by a flag #7319

Merged
Merged
Changes from 2 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -196,10 +196,12 @@ def initialize(self, all_options):

# Select a globally unique ID for the run, that sorts by time.
millis = int((self._run_timestamp * 1000) % 1000)
# run_uuid is used as a part of run_id and also as a trace_id for Zipkin tracing
run_uuid = uuid.uuid4().hex
run_id = 'pants_run_{}_{}_{}'.format(
time.strftime('%Y_%m_%d_%H_%M_%S', time.localtime(self._run_timestamp)),
millis,
uuid.uuid4().hex
run_uuid
)

info_dir = os.path.join(self.get_options().pants_workdir, self.options_scope)
@@ -229,7 +231,7 @@ def initialize(self, all_options):

self._all_options = all_options

return run_id
return (run_id, run_uuid)

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 12, 2019

Contributor

One way to avoid mixing up objects of the same type (especially strings) that I find useful sometimes is to use a datatype:

from future.utils import text_type

from pants.util.objects import datatype

class RunIds(datatype([('run_id', text_type), ('run_uuid', text_type)])):
  """Can add documentation in the docstring."""

You can then return RunIds(run_id=run_id, run_uuid=run_uuid) here (I prefer to use keyword arguments when the fields are the same type, like strings). In reporting.py, you can then:

    run_ids = run_tracker.initialize(all_options)
    run_dir = os.path.join(self.get_options().reports_dir, run_ids.run_id)
    # ...
    if trace_id is None:
      trace_id = run_ids.run_uuid

I personally find it difficult to the read the difference between run_id and run_uuid, so having the extra context can be helpful.

This comment has been minimized.

Copy link
@illicitonion

illicitonion Mar 13, 2019

Contributor

If we get to more than one call site, I would more strongly agree, but with one call site, this feels 50:50 useful vs overheady


def start(self, report, run_start_time=None):
"""Start tracking this pants run using the given Report.
@@ -69,7 +69,7 @@ def initialize(self, run_tracker, all_options, start_time=None):
TODO: See `RunTracker.start`.
"""

run_id = run_tracker.initialize(all_options)
run_id, run_uuid = run_tracker.initialize(all_options)
run_dir = os.path.join(self.get_options().reports_dir, run_id)

html_dir = os.path.join(run_dir, 'html')
@@ -112,6 +112,11 @@ 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 isn't set by a flag, use UUID from run_id
if trace_id is None:
trace_id = run_uuid

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. "
@@ -78,7 +78,7 @@ def start_workunit(self, workunit):
first_span = not self._workunits_to_spans
if first_span:
# If trace_id and parent_id are given create zipkin_attrs
if self.trace_id is not None:
if self.trace_id is not None and self.parent_id is not None:
zipkin_attrs = ZipkinAttrs(
trace_id=self.trace_id,
span_id=generate_random_64bit_string(),
@@ -88,6 +88,7 @@ def start_workunit(self, workunit):
)
else:
zipkin_attrs = create_attrs_for_span(
trace_id=self.trace_id, # use UUID from run_id as trace id

This comment has been minimized.

Copy link
@illicitonion

illicitonion Mar 13, 2019

Contributor

I would probably drop this comment; this code doesn't actually ensure that (code in another file does), and it's not ensured 100% of the time (people can also specify their own trace IDs with a flag)

This comment has been minimized.

Copy link
@cattibrie

cattibrie Mar 13, 2019

Author Contributor

there will be an error if the user will specify only trace_id flag, both trace_id and parent_id should be specified.

sample_rate=self.sample_rate, # Value between 0.0 and 100.0
)
self.trace_id = zipkin_attrs.trace_id
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.