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

Potentially incorrect number of read bytes in otelhttp client instrumentation #5087

Open
ash2k opened this issue Feb 13, 2024 · 1 comment
Open
Labels
area: instrumentation Related to an instrumentation package bug Something isn't working instrumentation: otelhttp

Comments

@ash2k
Copy link
Contributor

ash2k commented Feb 13, 2024

Description

From #4895 and #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 #4895 (comment). 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]
  • otelhttp version: [e.g. v0.14.0, 3c7face]

Steps To Reproduce

  1. Using this code ...
  2. Run ...
  3. See error ...

Expected behavior

Number of reported bytes read always matches the actual number of bytes read.

@ash2k ash2k added area: instrumentation Related to an instrumentation package bug Something isn't working instrumentation: otelhttp labels Feb 13, 2024
@dmathieu
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: instrumentation Related to an instrumentation package bug Something isn't working instrumentation: otelhttp
Projects
None yet
Development

No branches or pull requests

2 participants