Skip to content
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

Investigate query duration of backend #247

Closed
2opremio opened this issue Jul 11, 2024 · 7 comments
Closed

Investigate query duration of backend #247

2opremio opened this issue Jul 11, 2024 · 7 comments

Comments

@2opremio
Copy link
Contributor

After the transactions where moved from in-memory data structures to the backend, the average query duration has considerably increased.

This makes sense, but it has increased to 100ms, which is not the end of the world, but isn't great either.

See https://grafana.stellar-ops.com/d/ut9rEquVz/soroban-rpc?orgId=1&var-Datasource=Prometheus-kube001-prd&var-namespace=soroban-rpc-pubnet-prd

@2opremio
Copy link
Contributor Author

It seems to be caused by getHealth so, my bet is on GetLedgerRange(), which, according to #217 (comment) , it only takes microseconds.

So, something's up with either the benchmark of the environment.

@mollykarcher
Copy link
Contributor

so, my bet is on GetLedgerRange()

Isn't this called by other endpoints as well though? Why would we not see similar degradations on other endpoints if this was the culprit?

Semi-related; I guess this is a good thing rather than an issue, but do we understand why the performance of getEvents got a lot better due to this release?

image

@Shaptic
Copy link
Contributor

Shaptic commented Jul 11, 2024

From my perspective there are a handful of possible approaches here:

  1. caching the results of GetLedgerRange with eviction on ledger ingestion, which we've discussed
  2. trying a different query that might end up being faster
  3. adding a row to the table to store earliest ledger in addition to latest ledger instead of removing the table entirely? this is effectively the same as option (1) but in the DB instead of in memory (related: db: Remove latestledger key from meta table #233)

Maybe there are others.

Also, since it's only getHealth, it could also be related to Captive Core since it still uses the /info endpoint, right? Maybe #198 could fix it.

@mollykarcher mollykarcher assigned 2opremio and unassigned aditya1702 Jul 16, 2024
@mollykarcher mollykarcher moved this from Backlog to In Progress in Platform Scrum Jul 16, 2024
@mollykarcher mollykarcher added this to the platform sprint 49 milestone Jul 16, 2024
@2opremio
Copy link
Contributor Author

  1. caching the results of GetLedgerRange with eviction on ledger ingestion, which we've discussed

This is planned as per #233

  1. trying a different query that might end up being faster

I think the query is pretty fast already but, with catching, we should be able to only execute it once (we can book-keep it in memory after that)

Also, since it's only getHealth, it could also be related to Captive Core since it still uses the /info endpoint, right? Maybe #198 could fix it.

Ah, you are right. I forgot #198 is not it yet, that may be it!

@psheth9
Copy link
Contributor

psheth9 commented Jul 17, 2024

oh wait getHealth does not use /info endpoint right? so it should not be related to the improvement of #198 but regardless PR is ready to go and we should merge it ASAP

@jacekn jacekn self-assigned this Jul 19, 2024
@jacekn
Copy link
Contributor

jacekn commented Aug 19, 2024

@jacekn
Copy link
Contributor

jacekn commented Aug 22, 2024

The in-memory data structures will be removed in the future.
For the time being @2opremio gained some performance by changing page size so we're good.
I'll close this since there isn't much value in optimising something that will disappear in the future

@jacekn jacekn closed this as completed Aug 22, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in Platform Scrum Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

6 participants