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

Add support for reading B3 single header #165

Merged

Conversation

@ykitamura-mdsol
Copy link
Contributor

commented Oct 8, 2019

This PR is just to support the "reading" part of B3 single header (#126).
The writing part is not included.

@adriancole sorry for taking a long time but I like the single header!

@jcarres-mdsol @jfeltesse-mdsol

return new(parse_sampled_flags(b3_single_header)) if b3_single_header.size == 1

trace_id, span_id, flag, parent_span_id = b3_single_header.split('-')
new(trace_id: trace_id, span_id: span_id, parent_span_id: parent_span_id, **parse_sampled_flags(flag))

This comment has been minimized.

Copy link
@jfeltesse-mdsol

jfeltesse-mdsol Oct 8, 2019

Contributor

I think it's a bit wasteful to have this method create a new array (by calling split), then a new object and then in the caller call to_a which is going to create yet another array.

How about having a method getting the header as an input and return an array directly, without instantiating?

This comment has been minimized.

Copy link
@adriancole

adriancole Oct 8, 2019

Contributor

though I would say this is the least code approach I've ever seen :P

This comment has been minimized.

Copy link
@ykitamura-mdsol

ykitamura-mdsol Oct 8, 2019

Author Contributor

Updated in 2fef91b

@@ -0,0 +1,38 @@
module ZipkinTracer

This comment has been minimized.

Copy link
@jfeltesse-mdsol

jfeltesse-mdsol Oct 8, 2019

Contributor

how about adding the magic comment about frozen strings?

This comment has been minimized.

Copy link
@ykitamura-mdsol

ykitamura-mdsol Oct 8, 2019

Author Contributor

Added in f8d6a89

Copy link
Contributor

left a comment

Lookin good!

agree some performance work could be done, but I like it anyway

def called_with_zipkin_headers?
@called_with_zipkin_headers ||= B3_REQUIRED_HEADERS.all? { |key| @env.key?(key) }
end

private

B3_REQUIRED_HEADERS = %w(HTTP_X_B3_TRACEID HTTP_X_B3_SPANID).freeze
B3_OPT_HEADERS = %w(HTTP_X_B3_PARENTSPANID HTTP_X_B3_SAMPLED HTTP_X_B3_FLAGS).freeze
B3_SINGLE_HEADER = 'HTTP_B3'.freeze

This comment has been minimized.

Copy link
@adriancole

adriancole Oct 8, 2019

Contributor

easier to quote, ain't it :)

lib/zipkin-tracer/zipkin_b3_single_header_format.rb Outdated Show resolved Hide resolved
return new(parse_sampled_flags(b3_single_header)) if b3_single_header.size == 1

trace_id, span_id, flag, parent_span_id = b3_single_header.split('-')
new(trace_id: trace_id, span_id: span_id, parent_span_id: parent_span_id, **parse_sampled_flags(flag))

This comment has been minimized.

Copy link
@adriancole

adriancole Oct 8, 2019

Contributor

though I would say this is the least code approach I've ever seen :P

describe ZipkinTracer::B3SingleHeaderFormat do
let(:b3_single_header_format) { described_class.parse_from_header(b3_single_header) }

context 'child span' do

This comment has been minimized.

Copy link
@adriancole

adriancole Oct 8, 2019

Contributor

add some malformed tests? Ex in brave we use

"not-a-tumor"
"b970dafd-0d95-40aa-95d8-1d8725aebe40"

This comment has been minimized.

Copy link
@ykitamura-mdsol

ykitamura-mdsol Oct 8, 2019

Author Contributor

@adriancole In ruby client, we are doing malformed check in another class:

class TraceId128Bit < SpanId
HEX_REGEX_16 = /^[a-f0-9]{16}$/i
HEX_REGEX_32 = /^[a-f0-9]{32}$/i
MAX_SIGNED_I128 = (2 ** 128 / 2) -1
MASK = (2 ** 128) - 1
def self.from_value(v)
if v.is_a?(String) && v =~ HEX_REGEX_16
SpanId.new(v.hex)
elsif v.is_a?(String) && v =~ HEX_REGEX_32
new(v.hex)
elsif v.is_a?(Numeric)
new(v)
elsif v.is_a?(SpanId)
v
end
end

I think we don't need to have malformed tests here but what do you think?

ykitamura-mdsol and others added 3 commits Oct 8, 2019
Co-Authored-By: Adrian Cole <adriancole@users.noreply.github.com>
@adriancole

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

@jcarres-mdsol jcarres-mdsol merged commit 0a8be52 into openzipkin:master Oct 9, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ykitamura-mdsol ykitamura-mdsol deleted the ykitamura-mdsol:feature/read_b3_single_header branch Oct 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.