vendor: otel v0.56.0 / v1.31.0#49276
Conversation
|
|
||
| // TestOtelMeterLeak tests for a memory leak in the OTEL meter implementation. | ||
| // Once the fixed OTEL is vendored, this test will fail - the workaround | ||
| // and this test should be removed then. | ||
| func TestOtelMeterLeak(t *testing.T) { | ||
| meter := otel.Meter("foo") | ||
|
|
||
| var before runtime.MemStats | ||
| runtime.ReadMemStats(&before) | ||
|
|
||
| const counters = 10 * 1000 * 1000 | ||
| for i := 0; i < counters; i++ { | ||
| _, _ = meter.Int64Counter("bar") | ||
| } | ||
|
|
||
| var after runtime.MemStats | ||
| runtime.ReadMemStats(&after) | ||
|
|
||
| allocs := after.Mallocs - before.Mallocs | ||
| t.Log("Allocations:", allocs) | ||
|
|
||
| if allocs < 10 { | ||
| // TODO: Remove Workaround OTEL memory leak in cmd/dockerd/daemon.go | ||
| t.Fatal("Allocations count decreased. OTEL leak workaround is no longer needed!") | ||
| } | ||
| } |
There was a problem hiding this comment.
Wondering, maybe we should leave it, but inverse the check and fail to let us know in case it regresses in future?
There was a problem hiding this comment.
Yeah, was considering for a bit, but of course that's also assuming the exact same bug would come again I guess? We wouldn't catch issues in other areas of OTEL, or do you think those are less likely to happen?
There was a problem hiding this comment.
Yeah it's mostly for this exact bug. Although one case it would also catch is when something (like a vendored dependency in func init()) would override the global OTEL tracer provider.
There was a problem hiding this comment.
OK; updated the test to check for "more than 10" allocations, and added some comments to provide some context.
thaJeztah
force-pushed
the
vendor_otel
branch
from
c4e4114 to
d8f3fc7
Compare
Reverts otel workaround, added in cca7085, as it's no longer needed: === Failed === FAIL: cmd/dockerd TestOtelMeterLeak (0.64s) daemon_test.go:303: Allocations: 3 daemon_test.go:307: Allocations count decreased. OTEL leak workaround is no longer needed! We're keeping the test for now, so that we can check for possible regressions in the OTel dependencies. Co-authored-by: Derek McGowan <[email protected]> Signed-off-by: Derek McGowan <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
vendor_otel
branch
from
d8f3fc7 to
48e6b4e
Compare
|
Failure on windows looks like a flaky test, but let me kick it again |
2 participants
vendor: otel v0.56.0 / v1.31.0
Reverts otel workaround, added in cca7085,
as it's no longer needed:
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)