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

Conversation

Projects
None yet
4 participants
@cattibrie
Copy link
Contributor

commented Mar 6, 2019

Every Pants run has an ID (run_id, punts_run_...) and last part of this ID is a 32 characters hex string.
trace id in Zipkin traces is a 16 or 32 characters hex string. That means that Pants run_id can be used as a Zipkin trace id.
That may make it easier to correlate Zipkin traces with other places that we already log a run_id.

@blorente
Copy link
Contributor

left a comment

This is cool!

@illicitonion
Copy link
Contributor

left a comment

Looks great :) One minor re-work :) Thanks!


# If trace_id isn't set by a flag, use UUID from run_id
if trace_id is None:
_, _, trace_id = run_id.rpartition("_")

This comment has been minimized.

Copy link
@illicitonion

illicitonion Mar 6, 2019

Contributor

Rather than parsing this out (and assuming what the format of the run_id is), let's have RunTracker.initialize return a tuple of (run_id, run_uuid) and unpack it in this method

@cattibrie cattibrie force-pushed the cattibrie:etyurina/use_run_id_as_trace_id branch from c91a3fb to 0ace828 Mar 12, 2019

@cosmicexplorer
Copy link
Contributor

left a comment

Is there a reason we wouldn't want to always use the pants run id's uuid as the zipkin trace id (instead of having a separate trace id flag)? It seems like being able to get the zipkin trace id just by knowing the pants run id is a pretty useful thing to have.

@@ -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

@illicitonion
Copy link
Contributor

left a comment

One trivial comment, then ready to merge :)

@@ -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.

@illicitonion illicitonion merged commit a88855c into pantsbuild:master Mar 14, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.