opentelemetry-go-contrib
opentelemetry-go-contrib copied to clipboard
Potentially incorrect number of read bytes in otelhttp client instrumentation
Description
From https://github.com/open-telemetry/opentelemetry-go-contrib/issues/4895 and https://github.com/open-telemetry/opentelemetry-go-contrib/pull/5080:
Using atomic.Int64 will fix the data race but the logical race will still be there i.e. the number of read bytes will be incorrect because the reader goroutine might have not finished reading. My understanding is the value can only be read after Close() has been called on the body. And in that case a plain int64 should be fine.
I think this fix may lead to incorrect metric reported, see my comment here https://github.com/open-telemetry/opentelemetry-go-contrib/issues/4895#issuecomment-1931787280. This can be checked by having a reader for the body that is slow+a big body and a server that starts responding without consuming all of the incoming request's body.
Environment
- OS: [e.g. iOS]
- Architecture: [e.g. x86, i386]
- Go Version: [e.g. 1.15]
otelhttpversion: [e.g. v0.14.0, 3c7face]
Steps To Reproduce
- Using this code ...
- Run ...
- See error ...
Expected behavior
Number of reported bytes read always matches the actual number of bytes read.
The reason I didn't move the metrics to the reader is because we don't have access to the request in there, and it's therefore not trivial to add the metric attributes in there.
A proper fix also needs to include tests, when my quick one doesn't.