fix: lock accesses to encoder in json stdout exporter to prevent crash

[lizthegrey]

Which problem is this PR solving?

• locks accesses to encoder. fixes [crash] stdout json exporter crashes when many spans generated at once #2264

verified that

opentelemetry-go/exporters/stdout/stdoutmetric/metric.go

Line 162 in 79df6ec

func (e *metricExporter) marshal(v interface{}) ([]byte, error) {

does not suffer from this problem and thus doesn't also need to be fixed.

Short description of the changes

• locks accesses to encoder. fixes [crash] stdout json exporter crashes when many spans generated at once #2264

[codecov]

Codecov Report

Merging #2265 (1082e33) into main (00d8ca5) will decrease coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##            main   #2265     +/-   ##
=======================================
- Coverage   72.5%   72.4%   -0.1%     
=======================================
  Files        168     168             
  Lines      11872   11874      +2     
=======================================
- Hits        8609    8608      -1     
- Misses      3029    3031      +2     
- Partials     234     235      +1     

Impacted Files	Coverage Δ
exporters/stdout/stdouttrace/trace.go	76.4% <100.0%> (+0.9%)	⬆️
...s/otlp/otlptrace/internal/connection/connection.go	14.8% <0.0%> (-1.6%)	⬇️

[MadVikingGod]

Are we sure the problem is caused by concurrent access to the exporter's buffer? If it is we need to make sure all of our exporters are reentrant, because one of our assumptions of the Export() function is that the processor won't call it concurrently. The panic, that your example cuts off, is index out of range of the backing buffer. Specifically, it happens when the buffer inside the json encoder reslices it's buffer when it can't. I think we need to explore a bit more into what is causing this issue.

…

On Tue, Sep 28, 2021, 12:22 AM Robert Pająk ***@***.***> wrote: ***@***.**** approved this pull request. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#2265 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4XTGCZNXZ3EJYIJTORNELUEFGIDANCNFSM5E4HU2KA> .

[Aneurysm9]

That's a good point @MadVikingGod. It looks like the SimpleSpanProcessor is only taking a read lock on the exporter mutex, which wouldn't serialize access. See this discussion on its addition for more context.

I think that it should be safe to change that to an exclusive lock given that the SimpleSpanProcessor is expected to be synchronous. I don't think it hurts to also add the mutex around the stdouttrace exporter's use of the JSON encoder.

[lizthegrey]

That's a good point @MadVikingGod. It looks like the SimpleSpanProcessor is only taking a read lock on the exporter mutex, which wouldn't serialize access. See this discussion on its addition for more context.

I think that it should be safe to change that to an exclusive lock given that the SimpleSpanProcessor is expected to be synchronous. I don't think it hurts to also add the mutex around the stdouttrace exporter's use of the JSON encoder.

I fingered the change to the json encoder as the proximal change, because this is a regression from 0.20.0, and the simplespanprocessor hadn't changed since then. But I'm flexible as to how we fix this.

[evantorrie]

Root cause of the problem does appear to be that Go standard library's json.Encoder is not thread safe when using non-empty indentPrefix and/or indentValue.

It reuses the same indentBuf inside the Encoder struct rather than creating a new buffer each invocation to handle the pretty-printed JSON output.

@lizthegrey Does it work if you remove the pretty-printing option from the the stdouttrace exporter?

[lizthegrey]

Root cause of the problem does appear to be that Go standard library's json.Encoder is not thread safe when using non-empty indentPrefix and/or indentValue.

It reuses the same indentBuf inside the Encoder struct rather than creating a new buffer each invocation to handle the pretty-printed JSON output.

Yeah, this is documented here: golang/go#32957

@lizthegrey Does it work if you remove the pretty-printing option from the the stdouttrace exporter?

I'm sure it would, but Go gives guidance that you should not assume stdlib objects are thread-safe unless documented otherwise.

[Aneurysm9]

@lizthegrey can you merge in main and I'll get this merged? We can evaluate separately whether the simple span processor should be blocking as well.

[lizthegrey]

branch updated