sming commented 4 years ago

Implement a configurable ES result size

Please see Make suggest result size configurable #646 for a detailed discussion about the implementation. Especially the conversation about ES probably not being hit for 10K suggestions the vast majority of the time.

Use Case Resolved

I am a Grafana user
who wants the UI to be more responsive
so that I can work more efficiently and not get irritated

Use Case 2 Resolved

I am a Heroic developer
who wants to tighten-up/secure/protect Heroic against unreasonably large request suggestion limits, irrespective of their origin
so that I can rest easy at night

Design & Implementation Notes

a trivial new class NumSuggestionsLimit, objects of which are used by SuggestBackend implementations to determine the maximum number (i.e. limit) of suggestion entities to return.
said class will not allow a limit of more than 500. It will default to a limit of 50 if no limit is supplied by heroic.yaml for the respective Backend implementation.
when, for example, a call to tagSuggest() is made, if the request contains a limit, that limit is respected. If it does not, the Backend's limit is used.
Note that I also refactored a bunch of related code to help me understand it.

malish8632 commented 4 years ago

I've started looking into this. Lots of changes and need to wrap my mind around it. @sming did you look into failing tests?

sming commented 4 years ago

Overall the change LGTM. Did we try to run this version and verify the change locally against prod ES? As the change is sizable I think it would be helpful to compare results for random requests.

That is a very, very salient point @malish8632 - I haven't run it locally and recorded results. I will try that out once these pesky tests are working.

Just FYI, I'm not sure we're going to see any differences in normal operations because heroic datasource specifies a limit in the requests it sends to the Heroic API.

malish8632 commented 4 years ago

@sming may I suggest to run queries with higher limits in prod and with your new impl to see that it covers those cases.

sming commented 4 years ago

@sming may I suggest to run queries with higher limits in prod and with your new impl to see that it covers those cases.

When you say "in prod", do you mean create a prod heroic.yaml and run my branch locally to do tests? Or do you mean do the tests after it's deployed?

malish8632 commented 4 years ago

Yes that's what I meant. If you need help for say GUC production API config let me know. Thanks

sming commented 4 years ago

gotcha, will do.

On Wed, Aug 26, 2020 at 2:54 PM Sergey Rustamov notifications@github.com wrote:

Yes that's what I meant. If you need help for say GUC production API config let me know. Thanks

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/spotify/heroic/pull/685#issuecomment-681061513, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAKWXWXIGGOOFZNPOGKZ4RDSCVLABANCNFSM4QALEN6Q .

sming commented 4 years ago

I've started looking into this. Lots of changes and need to wrap my mind around it. @sming did you look into failing tests?

Forgot to say - just ask here on this PR with any questions you have, or hit me up on Slack. Yes, looking into build errors. CI != local dev currently...

sming commented 4 years ago

Hey @malish8632 , FYI I debugged heroic master v.s. this branch and :

## my branch
- a limit of 10 is respected (correct)
- 1000 limit request is changed to 250 (correct)

## master

- a limit of 10 is respected (correct)
- 1000 limit request is NOT changed to 250 (incorrect)

Hence this demonstrates that this PR is protecting Heroic from selfish/obnoxious/anti-social suggestion limits, should they be supplied in the request.

codecov[bot] commented 4 years ago

Codecov Report

Merging #685 into master will increase coverage by 0.41%. The diff coverage is 93.26%.

@@             Coverage Diff              @@
##             master     #685      +/-   ##
============================================
+ Coverage     53.79%   54.20%   +0.41%     
- Complexity     2965     3019      +54     
============================================
  Files           726      728       +2     
  Lines         19511    19624     +113     
  Branches       1288     1286       -2     
============================================
+ Hits          10495    10637     +142     
+ Misses         8564     8536      -28     
+ Partials        452      451       -1

Impacted Files	Coverage Δ	Complexity Δ
...ava/com/spotify/heroic/suggest/SuggestBackend.java	`50.00% <ø> (ø)`	`1.00 <0.00> (ø)`
...ify/heroic/suggest/memory/MemorySuggestModule.java	`73.52% <72.72%> (-1.48%)`	`4.00 <0.00> (ø)`
...gest/elasticsearch/ElasticsearchSuggestModule.java	`67.50% <76.47%> (-0.39%)`	`6.00 <0.00> (ø)`
...heroic/suggest/elasticsearch/SuggestBackendKV.java	`82.25% <89.00%> (+2.08%)`	`77.00 <39.00> (+16.00)`
...om/spotify/heroic/suggest/NumSuggestionsLimit.java	`93.75% <93.75%> (ø)`	`10.00 <10.00> (?)`
...m/spotify/heroic/suggest/memory/MemoryBackend.java	`93.75% <96.42%> (+0.55%)`	`42.00 <13.00> (+3.00)`
.../spotify/heroic/test/AbstractSuggestBackendIT.java	`99.50% <99.38%> (-0.50%)`	`47.00 <36.00> (+16.00)`	:arrow_down:
...rc/main/java/com/spotify/heroic/common/Series.java	`80.59% <100.00%> (-0.97%)`	`40.00 <3.00> (ø)`
...m/spotify/heroic/test/AbstractMetricBackendIT.java	`98.37% <100.00%> (+<0.01%)`	`29.00 <0.00> (ø)`
...va/com/spotify/heroic/test/TimestampPrepender.java	`100.00% <100.00%> (ø)`	`4.00 <4.00> (?)`
... and 12 more

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 666dbc1...63e90b0. Read the comment docs.

sming commented 4 years ago

Hi team, as per my comment above, this worked from the command line i.e. master allowed a request to specify 1,000 suggestions but my branch reduced that to the configured ceiling of 250.

Hence please re/review or approve or ignore as you feel fit - cheers. @malish8632 , @sjoeboo , @lmuhlha , @ao2017 , @samfadrigalan , adsail

spotify / heroic