BUG: Remove TerminateAfter from Elasticsearch/Opensearch query resulting in incomplete span count/list

[Jakob3xD]

Which problem is this PR solving?

Closes #4330

Short description of the changes

• Remove TerminateAfter from Elasticsearch/Opensearch query resulting in incomplete span count/list.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (5c3c20d) 97.07% compared to head (509c6f2) 97.08%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4336      +/-   ##
==========================================
+ Coverage   97.07%   97.08%   +0.01%     
==========================================
  Files         300      300              
  Lines       17728    17727       -1     
==========================================
+ Hits        17210    17211       +1     
+ Misses        416      415       -1     
+ Partials      102      101       -1     

Flag	Coverage Δ
unittests	97.08% <100.00%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
plugin/storage/es/spanstore/reader.go	100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[albertteoh]

It seems quite reasonable to me to remove the terminate after. IIUC, its primary use case is to protect shards from using a large amount of memory while also improving query times.
However, I feel query correctness should take precedence, especially if shard capacity can be configured.

I've just added a suggestion relating to testing.

[Jakob3xD]

@albertteoh @yurishkuro can we get this merged or is there anything left open?

[albertteoh]

For posterity, TerminateAfter was first introduced in this PR to address jaeger-query OOMing.

However, based on this SO answer and the ES docs, I don't see how TerminateAfter will help reduce memory pressure in jaeger-query although, iiuc, it can help reduce memory pressure in individual ES shards.

Maintainers: This bug fix is technically a breaking change, so I think we should report it as such in release notes.

@yurishkuro are you okay for this to be merged? I noticed you had some comments/questions in the linked issue.

[yurishkuro]

I think you meant this PR #1283 (#1202 was closed unmerged)

[yurishkuro]

This SO explanations also confirms that TerminateAfter is the wrong thing to do.