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

Public span fields cannot be safely accessed #58

Closed
elijahandrews opened this issue Apr 27, 2017 · 2 comments
Closed

Public span fields cannot be safely accessed #58

elijahandrews opened this issue Apr 27, 2017 · 2 comments
Labels
bug unintended behavior that has to be fixed
Milestone

Comments

@elijahandrews
Copy link
Contributor

elijahandrews commented Apr 27, 2017

We have a bunch of public fields on Span that cannot be safely accessed in multi-threaded code. These fields include Error, Name, Meta, and Metrics. We can't even ask the user to lock the span themselves before accessing these fields, as the mutex that protects them is private.

This is particular concerning for Meta and Metrics, as concurrent hash accesses can cause panics in newer versions of Go.

We should make these fields private and write public, thread-safe getters and setters to access this data.

@palazzem
Copy link
Contributor

palazzem commented Nov 8, 2017

@elijahandrews I'm going to change our public API since our Tracer should not expose some fields now that we're moving to OpenTracing. This issue should be addressed as a consequence.

@palazzem palazzem added the bug unintended behavior that has to be fixed label Nov 8, 2017
@palazzem
Copy link
Contributor

palazzem commented Dec 6, 2017

@elijahandrews the new OpenTracing API (#126) returns a Span interface that sets the underlying fields using the span.SetTag(k,v) method. In that case, a mutex will be used both for updating the Meta and other fields (Service, Resource, Type).

Since we're moving towards a OpenTracing-only API, we can consider this issue closed. Of course some pieces may be missing, but I think we should open other issues for those. Thank you!

@palazzem palazzem closed this as completed Dec 6, 2017
@palazzem palazzem added this to the 0.6.0 milestone Dec 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug unintended behavior that has to be fixed
Projects
None yet
Development

No branches or pull requests

3 participants