reporter: don't expire actively used executables #247
Conversation
Gandem
changed the title
| @@ -147,7 +151,7 @@ func NewOTLP(cfg *Config) (*OTLPReporter, error) { | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| executables.SetLifetime(1 * time.Hour) // Allow GC to clean stale items. | |||
| executables.SetLifetime(executableCacheLifetime) // Allow GC to clean stale items. | |||
There was a problem hiding this comment.
Shouldn't we also increase the size of this cache as 4096 seems small given we still haven't solved the core issue here, which is that items may be dropped from this cache (whether through time-based expiration or due to the LRU being full) and there's no control or guarantee as to when they'll be re-inserted.
I'd set the cache size to 16384 until we really solve the problem.
There was a problem hiding this comment.
No strong opinion on the cache size (4096 seems quite large to me).
Ideally, we should have metrics for expiration and eviction, then get numbers from production systems.
We can also make the cache sizes and lifetimes configurable.
And we can create an LRU wrapper that automatically resizes the LRUs, as suggested at #248 (comment)
Maybe better continue at #244 (comment)
There was a problem hiding this comment.
We should also not forget that (currently), the reporter implementation in this repository is just for demo/example purposes.
There was a problem hiding this comment.
No strong opinion on my side either - went ahead and bumped it to 16384 in bf83645
To make it less likely that LRU-full eviction evicts executable metadata for a valuable executable, see open-telemetry#247 (comment)
Ensure that actively used executable info is not expired from the cache, by extending its lifetime using
GetAndRefresh()Fixes #244