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

Switch to Zipkin v2 span format #141

Merged
merged 4 commits into from
Mar 8, 2019

Conversation

ykitamura-mdsol
Copy link
Contributor

Use the simplified Zipkin v2 span format to send traces to Zipkin server.

See openzipkin/zipkin#1499 for details

@cabbott @jcarres-mdsol @jfeltesse-mdsol @ssteeg-mdsol @piao-mdsol @adriancole

Copy link
Member

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

looks good. one thing to check for is that server spans always have timestamp and duration. In v2 format, an additional field span.shared is used to qualify if a server span uses same ID as what came in through headers

@@ -28,24 +28,19 @@ def resolved_ip_address?(ip_string)
ip_string.to_i.to_s == ip_string
end

def each_annotation(spans, &block)
def each_endpoint(spans, &block)
Copy link
Member

Choose a reason for hiding this comment

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

maybe add a comment about what this is doing? I'm guessing it is lazy parsing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this comment is explaining:

# Resolving hostnames is a very expensive operation. We want to store them raw in the main thread
# and resolve them in a different thread where we do not affect execution times.

@@ -55,12 +55,11 @@ def annotate_plugin(span, env, status, response_headers, response_body)

def trace!(span, zipkin_env, &block)
trace_request_information(span, zipkin_env)
span.record(Trace::Annotation::SERVER_RECV)
span.kind = Trace::Span::Kind::SERVER
Copy link
Member

Choose a reason for hiding this comment

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

is there a way to tell if the incoming span ID was shared (from b3 headers)? if so, add span.shared=true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated in 1139cc5

METHOD = "http.method".freeze
PATH = "http.path".freeze
STATUS = "http.status".freeze
LOCAL_COMPONENT = "lc".freeze
Copy link
Member

Choose a reason for hiding this comment

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

ls is no longer needed unless there's a value for it. sometimes we had "lc"->"" just to add an endpoint to a span.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems we are still using it in our library. Added TODO in 2cd021e

module Tag
METHOD = "http.method".freeze
PATH = "http.path".freeze
STATUS = "http.status".freeze
Copy link
Member

Choose a reason for hiding this comment

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

this is wrong, it should be http.status_code though possibly was always incorrect here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated d4b6254

@ykitamura-mdsol
Copy link
Contributor Author

It seems we always have duration in spans:

def close
@duration = to_microseconds(Time.now) - @timestamp
end

@jcarres-mdsol
Copy link
Collaborator

I am going to merge with the lc TODO. Moving to V2 is a huge step forward

@jcarres-mdsol jcarres-mdsol merged commit 4ff2a58 into openzipkin:master Mar 8, 2019
@codefromthecrypt
Copy link
Member

codefromthecrypt commented Mar 8, 2019 via email

@ykitamura-mdsol ykitamura-mdsol deleted the feature/format-v2 branch April 5, 2019 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants