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

[OpenTracing] allow setting service name after span is created #229

Merged

Conversation

lucaspimentel
Copy link
Member

@lucaspimentel lucaspimentel commented Dec 5, 2018

Allow a span's service name to be changed with ISpan.SetTag() after the span is created. Setting the service name before a span is created was already supported with ISpanBuilder.WithTag().
See #182.

@lucaspimentel lucaspimentel added the area:tests unit tests, integration tests label Dec 5, 2018
@lucaspimentel lucaspimentel self-assigned this Dec 5, 2018
@lucaspimentel lucaspimentel changed the title add unit tests for setting service name with OpenTracing [OpenTracing] allow setting service name after span is created Dec 5, 2018
@lucaspimentel lucaspimentel merged commit 9e05da8 into develop Dec 6, 2018
@lucaspimentel lucaspimentel deleted the lpimentel/add-opentracing-service-name-unit-tests branch December 6, 2018 16:15
public string ServiceName
{
get { return _context.ServiceName; }
internal set { _context.ServiceName = value; }
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm late to the party sorry, but why not let the non opentracing tracer change the service name too?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, there's nothing stopping us from allowing that as well. We just need to drop internal here. I was trying to fix the blocking issue only without changing anything else.

@lucaspimentel lucaspimentel removed the area:tests unit tests, integration tests label Feb 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants