Skip to content
This repository was archived by the owner on May 23, 2023. It is now read-only.

Provide a new Tag for PEER_HOST_IPV4#184

Merged
wu-sheng merged 1 commit intomasterfrom
wu-sheng/IPv4Tag
Sep 18, 2017
Merged

Provide a new Tag for PEER_HOST_IPV4#184
wu-sheng merged 1 commit intomasterfrom
wu-sheng/IPv4Tag

Conversation

@wu-sheng
Copy link
Copy Markdown
Member

I am creating a new pull request for #163. It is for #162 . And taken @yurishkuro 's suggestion.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-1.0%) to 73.09% when pulling b5ae56f on wu-sheng/IPv4Tag into 767f716 on master.

@wu-sheng wu-sheng merged commit f52e7c2 into master Sep 18, 2017
@wu-sheng wu-sheng deleted the wu-sheng/IPv4Tag branch September 18, 2017 02:12
* PEER_HOST_IPV4 records IPv4 host address of the peer.
*/
public static final IntTag PEER_HOST_IPV4 = new IntTag("peer.ipv4");
public static final IntOrStringTag PEER_HOST_IPV4 = new IntOrStringTag("peer.ipv4");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

By doing this there is not a way how to deprecate int part. Spec says that it's string https://github.com/opentracing/specification/blob/master/semantic_conventions.md.

@pavolloffay
Copy link
Copy Markdown
Member

PR open-review-merge in 4 hours (on Sunday) does not leave any time for other people to look/review/comment.

@wu-sheng
Copy link
Copy Markdown
Member Author

@pavolloffay Because this is a pr last several months... and @yurishkuro think this is a bad time to break the API, and I agree him.

So, by @yurishkuro suggestion (good one), I did in this pr. Origin pr is this: #163. The discussion ends 3 days ago. I delayed my work... because of releasing skywalking 3.2

I known the spec said string, but we have released so many versions. We must consider that.

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.

4 participants