When serializing SpanModel, if there is any external modification of TAGS, it will lead to panic, add RLock before calling Send to prevent this error.

[php-lsys]

When serializing SpanModel, if there is any external modification of TAGS, it will lead to panic, add RLock before calling Send to prevent this error.
issues:#217

[jcchavezs]

LGTM

[codecov]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (87e3b85) 66.83% compared to head (c8522fb) 69.29%.
Report is 2 commits behind head on master.

Files	Patch %	Lines
span_implementation.go	66.66%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #218      +/-   ##
==========================================
+ Coverage   66.83%   69.29%   +2.45%     
==========================================
  Files          36       36              
  Lines        1770     2501     +731     
==========================================
+ Hits         1183     1733     +550     
- Misses        542      723     +181     
  Partials       45       45              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[codefromthecrypt]

My guess is that from a performance point of view, locking inside functions used to report will have a negligible perf impact, considering the lock is per-span. We've had to do similar in brave (java) for sometimes unexplainable external modifications (e.g. instrumentation still running while the span is reporting, something @reta has been looking at). Locking externally isn't particularly easy for outsiders to do.

In the java project, we have some race condition tests, and I'm not sure they exist here. This means we're left with visual commentary such as mine above or linters. It would be nice to add mutex tests or otherwise tests that would fail on go test -race. Maybe other go libraries will have some.

Meanwhile the lack of these tests isn't new. However, the code metrics have gone a little south. If we merge this, probably we should pay down some coverage to make up for it.