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

Conform zipkin traceId, spanId, parentId specs #604

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

praxist
Copy link
Contributor

@praxist praxist commented Jul 22, 2021

Using v1 API defined in https://github.com/openzipkin/zipkin-api/blob/7692ca7be4dc3be9225db550d60c4d30e6e9ec59/zipkin-api.yaml

traceId to 32 character lowercase hex string
spanId to 16 character lowercase hex string
parentId to be absent for root span

Currently required changes to prove out a grafana-agent + tempo tracing architecture.

@praxist praxist requested a review from a team as a code owner July 22, 2021 01:46
Using v1 API defined in https://github.com/openzipkin/zipkin-api/blob/7692ca7be4dc3be9225db550d60c4d30e6e9ec59/zipkin-api.yaml

traceId to 32 character lowercase hex string
spanId to 16 character lowercase hex string
parentId to be absent for root span
@@ -124,8 +124,9 @@ def new(cls) -> "TraceInfo":
with any upstream requests.

"""
trace_id = str(random.getrandbits(64))
return cls(trace_id=trace_id, parent_id=None, span_id=trace_id, sampled=None, flags=None)
trace_id = "{0:0{1}x}".format(random.getrandbits(128), 32)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

padded string with leading 0's to reach 32 characters - necessary in case hex string comes up short.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
trace_id = "{0:0{1}x}".format(random.getrandbits(128), 32)
trace_id = f"{random.getrandbits(128):032x}"

Parameterizing the 32 makes it quite hard to read the format string, and using f-strings makes this a little easier to understand as well.

@fishy
Copy link
Member

fishy commented Jul 22, 2021

This needs to be opt-in and default to false, as the older version of baseplate.py/go library will reject hex ids.

We already implemented the opt-in in .go, see the comment there: https://pkg.go.dev/github.com/reddit/baseplate.go/tracing#TracerConfig.UseUUID

@spladug spladug self-requested a review July 22, 2021 16:48
Copy link
Contributor

@spladug spladug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! It sounds like we're going ahead with the hex format so this looks good but as @fishy pointed out we'll need this to be opt-in for now.

@@ -124,8 +124,9 @@ def new(cls) -> "TraceInfo":
with any upstream requests.

"""
trace_id = str(random.getrandbits(64))
return cls(trace_id=trace_id, parent_id=None, span_id=trace_id, sampled=None, flags=None)
trace_id = "{0:0{1}x}".format(random.getrandbits(128), 32)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
trace_id = "{0:0{1}x}".format(random.getrandbits(128), 32)
trace_id = f"{random.getrandbits(128):032x}"

Parameterizing the 32 makes it quite hard to read the format string, and using f-strings makes this a little easier to understand as well.

trace_id = str(random.getrandbits(64))
return cls(trace_id=trace_id, parent_id=None, span_id=trace_id, sampled=None, flags=None)
trace_id = "{0:0{1}x}".format(random.getrandbits(128), 32)
span_id = "{0:0{1}x}".format(random.getrandbits(64), 16)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
span_id = "{0:0{1}x}".format(random.getrandbits(64), 16)
span_id = f"{random.getrandbits(64):016x}"

@@ -279,7 +279,9 @@ def _to_span_obj(
"binaryAnnotations": binary_annotations,
}

span["parentId"] = self.span.parent_id or 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. This means we don't send a parentId at all if it's undefined. Is that right? No default?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants