Skip to content

Handle ports > 32k in Zipkin JSON#488

Merged
yurishkuro merged 2 commits intomasterfrom
fix-467
Oct 23, 2017
Merged

Handle ports > 32k in Zipkin JSON#488
yurishkuro merged 2 commits intomasterfrom
fix-467

Conversation

@yurishkuro
Copy link
Copy Markdown
Member

Resolves #467

Signed-off-by: Yuri Shkuro <ys@uber.com>
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling 5e4d4f8 on fix-467 into c32f960 on master.

@yurishkuro yurishkuro changed the title Handle ports < 32k in Zipkin JSON Handle ports > 32k in Zipkin JSON Oct 23, 2017
return
}
if want, have := 1, len(handler.zipkinSpansHandler.(*mockZipkinHandler).getSpans()); want != have {
if want, have := expecting, len(handler.getSpans()); want != have {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is "want" necessary? just do

if have := ....; expected != have {

}
port := e.Port
if port >= (1 << 15) {
// Zipkin.thrift defines port as i16, so values between (2^16 and 2^16-1) must be encoded as negative
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

2^15 and 2^16-1?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

what's the question?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

oh, yeah

Signed-off-by: Yuri Shkuro <ys@uber.com>
@ghost ghost assigned yurishkuro Oct 23, 2017
@ghost ghost added the review label Oct 23, 2017
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling cd5ea64 on fix-467 into c32f960 on master.

@yurishkuro yurishkuro merged commit a7f203b into master Oct 23, 2017
@ghost ghost removed the review label Oct 23, 2017
@yurishkuro yurishkuro deleted the fix-467 branch October 23, 2017 19:41
isaachier pushed a commit to isaachier/jaeger that referenced this pull request Nov 1, 2017
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