Expose cache ttl for es span writer index+service

[necrolyte2]

Which problem is this PR solving?

• Unable to specify a cache duration for the service or index caches
• Our use case requires that we can set these

Short description of the changes

• Added ability to pass ttl duration for both service and index caches for the elastic spanstore writer
• If not specified it will use the previous defaults of 12h and 48h

[codecov]

Codecov Report

Merging #2737 (b8afaff) into master (fe2a8ab) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2737      +/-   ##
==========================================
+ Coverage   95.83%   95.84%   +0.01%     
==========================================
  Files         217      217              
  Lines        9650     9656       +6     
==========================================
+ Hits         9248     9255       +7     
  Misses        331      331              
+ Partials       71       70       -1     

Impacted Files	Coverage Δ
plugin/storage/es/spanstore/writer.go	100.00% <100.00%> (ø)
cmd/collector/app/server/zipkin.go	73.07% <0.00%> (-3.85%)	⬇️
cmd/query/app/static_handler.go	96.77% <0.00%> (+1.61%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fe2a8ab...b8afaff. Read the comment docs.

[yurishkuro]

• please add test coverage
• how would someone use this functionality? it is not exposed via CLI flags

[yurishkuro]

Suggested change

	ServiceCacheDuration string
	IndexCacheDuration string
	ServiceCacheDuration time.Duration
	IndexCacheDuration time.Duration

[necrolyte2]

You cannot check if a time.Duration is set or not since it would be 0, but you can check if a string is empty which is why I went the path I did there.

Anybody directly using the writer in their own code can use it and if later somebody wants to plumb in the ability for a flag for the apps to be able to set these could do that in a separate PR

I'm not sure how to test this since everything is in a private property and I cannot check if they are set in a test

[yurishkuro]

What's wrong with 0? 0 is not a valid value for these parameters, so it works perfectly well as "unset" value.

Tests run in the private package namespace and can test those values.

[necrolyte2]

0 is a valid value and would indicate caching forever so you still would not be able to differentiate from cache forever and not being set. Unless you want to assume that nobody would ever want to cache forever(which could be a valid thing, but I would rather be able to know for sure)

I did figure out how to test and just pushed those

[yurishkuro]

0 is a valid value and would indicate caching forever

Technically, for the LRU cache that is used under the hood, the ttl=0 is a valid param. But in the context of the specific caching scenarios here (index and service names) I do not believe 0 is a valid value, because these caches grow over time. So we can use 0 as "undefined". If people want to cache "forever" they can still do it by providing a very long duration.

[necrolyte2]

Sounds good. I can make the change

[yurishkuro]

If you really want to support "undefined" then the other alternative is to use a pointer.

[yurishkuro]

Thanks!