cmd/dockerd: Add workaround for OTEL meter leak#48690
Conversation
vvoland
added
status/2-code-review
kind/bugfix
vvoland
force-pushed
the
otel-meter-leak
branch
2 times, most recently
from
f867dd9 to
aba5f3c
Compare
cmd/dockerd/daemon.go
Outdated
|
|
||
| // Workaround OTEL memory leak | ||
| // See: https://github.com/open-telemetry/opentelemetry-go-contrib/issues/5190 | ||
| // The need for this workaround is checked by the TestOtelMeterLeak test | ||
| // TODO: Remove this workaround after upgrading to v1.30.0 | ||
| otel.SetMeterProvider(noop.MeterProvider{}) | ||
|
|
There was a problem hiding this comment.
Curious if this should be "as early as possible" in the chain (as a default, which can be overridden by "whatever" will override).
Would this fix still work if we'd put it (e.g.) in newDaemonCommand (assuming setting it in main() wouldn't make it easy to test)
There was a problem hiding this comment.
It would work, but I think it's much better to have it in the same place as other OTEL related stuff. If we'd like to actually override it, we can just remove it as it won't be needed anyway.
There was a problem hiding this comment.
Yeah, it's also that we don't always know where things are overridden (at least I recall some of the ugly bits where BuildKit setup things in an init().
but I think it's much better to have it in the same place as other OTEL related stuff
Yeah, even wondering if there's bits around that that we should also set up earlier (making sure that we trace as much as possible, starting with main ?)
There was a problem hiding this comment.
I can move it to main, so at least it stops the leak earlier. Although that shouldn't be the case as AFAIK the only meter counting on our side come from otelhttp.
I don't think it's reasonable for us to consider dependencies silently override the meter provider without our deliberate decision though.
vvoland
force-pushed
the
otel-meter-leak
branch
from
aba5f3c to
68366b8
Compare
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM, thank you!
Assuming this fixes the issue, happy to hear from others of course.
(left some minor nits)
OTEL meter implementation has a memory leak issue which causes each meter counter invocation to create a new instrument when the meter provider is not set. Also add a test, which will fail once a fixed OTEL is vendored. Signed-off-by: Paweł Gronowski <[email protected]>
vvoland
force-pushed
the
otel-meter-leak
branch
from
68366b8 to
cca7085
Compare
|
I added cherry-pick labels for older releases as well, but not sure if those need it, so verify if needed before backporting (it's more as a "reminder") @cpuguy83 @austinvazquez @corhere |
4 participants
otelhttpwhen no Meter Provider is configured. open-telemetry/opentelemetry-go-contrib#5190OTEL meter implementation has a memory leak issue which causes each meter counter invocation to create a new instrument when the meter provider is not set.
Also add a test, which will fail once a fixed OTEL is vendored.
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)