Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

Tighten Span#isRPC check #100

Merged
merged 1 commit into from
Nov 30, 2016
Merged

Tighten Span#isRPC check #100

merged 1 commit into from
Nov 30, 2016

Conversation

vprithvi
Copy link
Contributor

@vprithvi vprithvi commented Nov 30, 2016

  • Span#isRPC is set specifically for server or client spans. Other values for span.kind don't affect isRPC.

- Span#isRPC is set specifically for server or client spans.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 78.704% when pulling 5160de4 on fix_span_type into dcfde78 on master.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Please change the PR description to better reflect the change (looks like it's fixing a bug where server spans were not identified as RPC?)

@vprithvi
Copy link
Contributor Author

@yurishkuro Not really, previously we set isRPC to true simply when a span.kind was present. Now we tighten the check to only set it to true when span.kind is server or client. I updated the commit message to clarify this.

@vprithvi vprithvi changed the title Make Span#isRPC more readable Tighten Span#isRPC check and make it more readable Nov 30, 2016
@yurishkuro
Copy link
Member

Ah, ok, good catch.

@yurishkuro
Copy link
Member

But the PR description is still confusing - you're changing functionality, being "more readable" is much less irrelevant.

@vprithvi vprithvi changed the title Tighten Span#isRPC check and make it more readable Tighten Span#isRPC check Nov 30, 2016
@vprithvi
Copy link
Contributor Author

Agree, shortened the message

@vprithvi vprithvi merged commit 2bcca42 into master Nov 30, 2016
@vprithvi vprithvi deleted the fix_span_type branch November 30, 2016 23:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants