-
Notifications
You must be signed in to change notification settings - Fork 3
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
improvement: Add tags to span as SpanOptions #54
improvement: Add tags to span as SpanOptions #54
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this lgtm but will give the others some time to ack
wtracing/span.go
Outdated
@@ -74,6 +75,7 @@ type SpanOptionImpl struct { | |||
RemoteEndpoint *Endpoint | |||
ParentSpan *SpanContext | |||
Kind Kind | |||
Tags []Tag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason this is a []Tag
instead of the map[string]string
defined on the model? Using a map here would prevent the construction in span.go and embeds the logic of "most recent will prevail" since you can just overwrite existing values if they exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could just as easily be a map here. I was trying to consider what would be most performant but I think using a map in the SpanOption is just as good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to be a map from the start. Removed the new unnecessary tag.go file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Before this PR
wtracing did not use zipkin span tags
After this PR
==COMMIT_MSG==
Enable span tags as SpanOptions
==COMMIT_MSG==
This change is