-
-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[bazarr] Add bounded concurrency to episodes endpoint #238
Conversation
@rtrox ill run this later tonight to get some benchmark numbers for comparision |
@phyzical - that'd be great, I don't actually have a bazarr instance to test against >.< I'm reasonably confident from the unittests, but mind comparing the metric values too and just double checking that there aren't any logic bugs in the counting? |
@phyzical - one more note -- in lidarr & sonarr we have a similar pattern, where a few specific metrics require us to send an API call per object. In both places, we hide these metrics behind the |
i did have that if sitting there ready for heavy lifting, but as you've guessed it was kinda of all or nothing. The other reason i didn't bother was that i thought i had a fairly large sample size (3k+ series) and i was able to get it all within 30 seconds, which was more than enough for my usecase (checking a dashboard once in a blue moon) i personally think we should maybe be suggesting that these exporters are configured for longer intervals i.e once every 5-10 minutes anyway, that way if it is an expensive scrape it doesn't matter as much. the only outlier i can think of is alerts tied to disk space/health check, but even then id say finding out at 5-10 minutes for hobby stuff is still decent. But ill give it a test still because if its does speed things up why not! |
Yea, that makes sense. The only thing to keep in mind is that grafana's prometheus integration's max_source_resolution is somewhat low (I think it's a minute, but don't quote me on that). So if we decided to give that advice, we'll need to document the process of adding That said, breaking this up was probably necessary anyway, in the linked issue the url alone was near 100K, which means the memory needed to deserialize the response was definitely going to be a bit crazy >.<. |
yeah i saw the size of the log in the 20 mb range 😆 TBH ive been grafanaing for about 4 months so good to know the grafana limitation. But i think another part of the issue here is also that we assume each request should perform a new scrape, most of the exporters ive used tend to work with a scrape interval internally and cache the result for the prometheus instance to just get served some cold hard data. Not saying that should be done in this pr. But something that should be considered if it hasnt already. (i assume itll be a "oh its on the list" and there just wasnt enough justification) |
Believe it or not, caching and scrape intervals are actually anti-patterns in Prometheus exporters 🙃 From the exporter instrumentation docs:
We have a couple places that we do cache - in prowlarr, for example, as prowlarr exposes stats in an odd way which requires us to calculate and cache deltas to create accurate counts. Otherwise, we generally avoid caching and stick with synchronous calls. |
yeah that makes sense. in most cases just adjusting interval and scrape times would suffice (has for me). And its probably like anything prometheus keep the rules simple and locked down and it should just work out the box. The caching is it a simple toggleable feature or is it fairly deep? it does feel like an edgecase at this point i would personally consider my library the large end but this other users has double what i do so 😆 (no saying it shouldnt be a feature but should depend on how complicated it becomes to support optional caching) i did actually try asyncing the calls originally too but found it made things slower and i think i'm seeing the same during testing?
|
so yeah its actually slower, which is something i noticed when i tried batching it originally, granted i didn't try hard to workout why i did my best to be fair too. 1 req at a time randomly switching between who was next and no prometheus running causing additional random load. Logic wise its the same though |
It's tied into the prowlarr logic, but it's also not terribly complicated. You can see the indexerStatCache here. That said, I only did caching there due to the shape of the data returned by prowlarr (their API accepts a time range, and gives aggregated stats by day. So we have to cache last seen values to calculate differentials).
It's probably just the sheer number of calls. I set the default concurrency limit to If it's still not faster, I think I'll still merge it, it looks like it's about a ~10% regression, which is likely worth it to reduce the size of the API requests. But if we can't actually get faster, I'll likely put the iterative call behind (@onedr0p feel free to veto me on that) |
Sounds good to me :) |
@rtrox ill give a few more tests around But i think leveraging "EnableAdditionalMetrics" for sync v async i think is fair, though may be a bit misleading But i think if it does actually allow users to use the implementation i think its a good idea. |
That's not quite what I mean -- either way I'll want to switch to the threaded requests here, I'm not super comfortable leaving the episode history calls in place as is, the URLs are too large for me to be comfortable with on the wire, and the memory required for the responses is concerning. So I'm actually ok with a wall clock regression to move to the threaded model where request size is bounded. What I mean is -- if we can't find defaults that let us reduce the wall clock time, I plan to put that call and the metrics it generates behind So let's see if we can find some defaults that boost wall clock time, otherwise I'll start some refactoring to pull more metrics from the |
Ah yep, sorry i get you now. i think your safe to just chuck both the history blocks behind the flag, i believe i didn't just cause i was focusing on timings and not ram usage. I don't think you can get the data found in history in the main api call (which is why i started using it) but i could be wrong. |
So almost all the attempts floated around the 50mb range until i increased the batch size which makes sense. But i think the "recommendation" should be increase batch to reduce ram usage at the cost of longer requests, increase batch size for faster responses at the cost of more ram usage. personally i think ill just configure it to, have a batch size of 300-500 and use the default concurrency. @rtrox feel free to list some combos of vars you want the metrics of if it helps at all, and i will do some more tests |
Thanks @phyzical, great stuff! Yea, I'd suspect the same. I'd guess either Bazarr, SQLite, or query performance as the issue, but this is probably good enough, 20-30 seconds at your library size seems reasonably solid. So to land this, I think two updates:
I should be be able to get to that over the long weekend so we can land this early next week. |
sounds good to me 👍 |
Signed-off-by: Russell <russell@troxel.io>
Signed-off-by: Russell <russell@troxel.io>
Signed-off-by: Russell <russell@troxel.io>
Signed-off-by: Russell <russell@troxel.io>
…tionalMetrics Signed-off-by: Russell Troxel <russell@troxel.io>
4df37c6
to
84760de
Compare
Sorry, took a bit to get back to this PR, life got hectic >.< I think this should be ready for final review+merge. |
no worries, ill give it a scan and test agaisnt my instance when i can |
ah sorry i forgot then the holidays happened, i will give this a look this week |
was able to do a proper benchmark comparison diff wise for information it is exactly the same with the 👍 |
@onedr0p when you have time, i think this is good to be merged |
….1@60cf3d4 by renovate (#17725) This PR contains the following updates: | Package | Update | Change | |---|---|---| | [ghcr.io/onedr0p/exportarr](https://togithub.com/onedr0p/exportarr) | patch | `v1.6.0` -> `v1.6.1` | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>onedr0p/exportarr (ghcr.io/onedr0p/exportarr)</summary> ### [`v1.6.1`](https://togithub.com/onedr0p/exportarr/releases/tag/v1.6.1) [Compare Source](https://togithub.com/onedr0p/exportarr/compare/v1.6.0...v1.6.1) ##### What's Changed - Add tests for Radarr by [@​rtrox](https://togithub.com/rtrox) in [onedr0p/exportarr#234 - Add tests for Sonarr by [@​rtrox](https://togithub.com/rtrox) in [onedr0p/exportarr#235 - Add tests for Readarr by [@​rtrox](https://togithub.com/rtrox) in [onedr0p/exportarr#236 - doc(README): remove typo by [@​Deep145757](https://togithub.com/Deep145757) in [onedr0p/exportarr#240 - chore(deps): update golang docker tag to v1.21.4 by [@​renovate](https://togithub.com/renovate) in [onedr0p/exportarr#242 - chore(deps): update actions/setup-go action to v5 by [@​renovate](https://togithub.com/renovate) in [onedr0p/exportarr#248 - chore(deps): update golang docker tag to v1.21.5 by [@​renovate](https://togithub.com/renovate) in [onedr0p/exportarr#247 - chore(deps): update actions/checkout digest to [`b4ffde6`](https://togithub.com/onedr0p/exportarr/commit/b4ffde6) by [@​renovate](https://togithub.com/renovate) in [onedr0p/exportarr#233 - \[bazarr] Add bounded concurrency to episodes endpoint by [@​rtrox](https://togithub.com/rtrox) in [onedr0p/exportarr#238 - chore(deps): update golangci/golangci-lint-action action to v3 by [@​renovate](https://togithub.com/renovate) in [onedr0p/exportarr#257 - fix: add `gomodTidy` option to renovate by [@​rtrox](https://togithub.com/rtrox) in [onedr0p/exportarr#258 - fix(deps): update module github.com/gookit/validate to v1.5.2 by [@​renovate](https://togithub.com/renovate) in [onedr0p/exportarr#253 - chore(deps): update actions/setup-go action to v5 by [@​renovate](https://togithub.com/renovate) in [onedr0p/exportarr#256 - chore(deps): update actions/checkout digest to [`b4ffde6`](https://togithub.com/onedr0p/exportarr/commit/b4ffde6) by [@​renovate](https://togithub.com/renovate) in [onedr0p/exportarr#255 - fix(deps): update module github.com/prometheus/client_golang to v1.18.0 by [@​renovate](https://togithub.com/renovate) in [onedr0p/exportarr#250 - fix(deps): update module golang.org/x/sync to v0.6.0 by [@​renovate](https://togithub.com/renovate) in [onedr0p/exportarr#220 - fix(deps): update module github.com/spf13/cobra to v1.8.0 by [@​renovate](https://togithub.com/renovate) in [onedr0p/exportarr#241 - fix(deps): update golang.org/x/exp digest to [`1b97071`](https://togithub.com/onedr0p/exportarr/commit/1b97071) by [@​renovate](https://togithub.com/renovate) in [onedr0p/exportarr#243 - chore(deps): update golang docker tag to v1.21.6 by [@​renovate](https://togithub.com/renovate) in [onedr0p/exportarr#251 - fix([#​252](https://togithub.com/onedr0p/exportarr/issues/252)): Handle an empty Server Stat map returned from Sab. by [@​rtrox](https://togithub.com/rtrox) in [onedr0p/exportarr#259 ##### New Contributors - [@​Deep145757](https://togithub.com/Deep145757) made their first contribution in [onedr0p/exportarr#240 **Full Changelog**: onedr0p/exportarr@v1.6.0...v1.6.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 10pm on monday" in timezone Europe/Amsterdam, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xNTMuMiIsInVwZGF0ZWRJblZlciI6IjM3LjE1My4yIiwidGFyZ2V0QnJhbmNoIjoibWFzdGVyIn0=-->
….1@60cf3d4 by renovate (truecharts#17725) This PR contains the following updates: | Package | Update | Change | |---|---|---| | [ghcr.io/onedr0p/exportarr](https://togithub.com/onedr0p/exportarr) | patch | `v1.6.0` -> `v1.6.1` | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>onedr0p/exportarr (ghcr.io/onedr0p/exportarr)</summary> ### [`v1.6.1`](https://togithub.com/onedr0p/exportarr/releases/tag/v1.6.1) [Compare Source](https://togithub.com/onedr0p/exportarr/compare/v1.6.0...v1.6.1) ##### What's Changed - Add tests for Radarr by [@&truecharts#8203;rtrox](https://togithub.com/rtrox) in [onedr0p/exportarr#234 - Add tests for Sonarr by [@&truecharts#8203;rtrox](https://togithub.com/rtrox) in [onedr0p/exportarr#235 - Add tests for Readarr by [@&truecharts#8203;rtrox](https://togithub.com/rtrox) in [onedr0p/exportarr#236 - doc(README): remove typo by [@&truecharts#8203;Deep145757](https://togithub.com/Deep145757) in [onedr0p/exportarr#240 - chore(deps): update golang docker tag to v1.21.4 by [@&truecharts#8203;renovate](https://togithub.com/renovate) in [onedr0p/exportarr#242 - chore(deps): update actions/setup-go action to v5 by [@&truecharts#8203;renovate](https://togithub.com/renovate) in [onedr0p/exportarr#248 - chore(deps): update golang docker tag to v1.21.5 by [@&truecharts#8203;renovate](https://togithub.com/renovate) in [onedr0p/exportarr#247 - chore(deps): update actions/checkout digest to [`b4ffde6`](https://togithub.com/onedr0p/exportarr/commit/b4ffde6) by [@&truecharts#8203;renovate](https://togithub.com/renovate) in [onedr0p/exportarr#233 - \[bazarr] Add bounded concurrency to episodes endpoint by [@&truecharts#8203;rtrox](https://togithub.com/rtrox) in [onedr0p/exportarr#238 - chore(deps): update golangci/golangci-lint-action action to v3 by [@&truecharts#8203;renovate](https://togithub.com/renovate) in [onedr0p/exportarr#257 - fix: add `gomodTidy` option to renovate by [@&truecharts#8203;rtrox](https://togithub.com/rtrox) in [onedr0p/exportarr#258 - fix(deps): update module github.com/gookit/validate to v1.5.2 by [@&truecharts#8203;renovate](https://togithub.com/renovate) in [onedr0p/exportarr#253 - chore(deps): update actions/setup-go action to v5 by [@&truecharts#8203;renovate](https://togithub.com/renovate) in [onedr0p/exportarr#256 - chore(deps): update actions/checkout digest to [`b4ffde6`](https://togithub.com/onedr0p/exportarr/commit/b4ffde6) by [@&truecharts#8203;renovate](https://togithub.com/renovate) in [onedr0p/exportarr#255 - fix(deps): update module github.com/prometheus/client_golang to v1.18.0 by [@&truecharts#8203;renovate](https://togithub.com/renovate) in [onedr0p/exportarr#250 - fix(deps): update module golang.org/x/sync to v0.6.0 by [@&truecharts#8203;renovate](https://togithub.com/renovate) in [onedr0p/exportarr#220 - fix(deps): update module github.com/spf13/cobra to v1.8.0 by [@&truecharts#8203;renovate](https://togithub.com/renovate) in [onedr0p/exportarr#241 - fix(deps): update golang.org/x/exp digest to [`1b97071`](https://togithub.com/onedr0p/exportarr/commit/1b97071) by [@&truecharts#8203;renovate](https://togithub.com/renovate) in [onedr0p/exportarr#243 - chore(deps): update golang docker tag to v1.21.6 by [@&truecharts#8203;renovate](https://togithub.com/renovate) in [onedr0p/exportarr#251 - fix([#&truecharts#8203;252](https://togithub.com/onedr0p/exportarr/issues/252)): Handle an empty Server Stat map returned from Sab. by [@&truecharts#8203;rtrox](https://togithub.com/rtrox) in [onedr0p/exportarr#259 ##### New Contributors - [@&truecharts#8203;Deep145757](https://togithub.com/Deep145757) made their first contribution in [onedr0p/exportarr#240 **Full Changelog**: onedr0p/exportarr@v1.6.0...v1.6.1 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 10pm on monday" in timezone Europe/Amsterdam, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xNTMuMiIsInVwZGF0ZWRJblZlciI6IjM3LjE1My4yIiwidGFyZ2V0QnJhbmNoIjoibWFzdGVyIn0=-->
Description of the change
This PR adds pagination to collection from the
/api/episodes
endpoint in the bazarr collector. This pagination uses bounded concurrency, sending no more than--series-batch-concurrency
(default 10) batches to the bazarr server at a time, and asking for no more than--series-batch-size
(default 50) seriesIds at a time.fixes #237
A test image is available at
ghcr.io/rtrox/exportarr:episodes-concurrency
.Benefits
Allows retrieval of series metrics from bazarr instances with a large number of serieses.
Possible drawbacks
More goroutines?
Applicable issues
Additional information