✨ Cache list service request in catalog #2874

sanderegg · 2022-03-08T13:20:25Z

What do these changes do?

This PR follows #2869 and the idea is to provide caching of the request listing services from the catalog using for now a simple cache mechanism through aiocache library.
This library could allow caching through Redis if need be. for now it caches in memory for a 30 seconds time.

The requests per seconds doubles using that mechanism, during the 30seconds the cache lives.

Without cache:

------------------------------------------------------------------------------------------------ benchmark: 2 tests ------------------------------------------------------------------------------------------------
  Name (time in ms)                             Min                   Max                  Mean             StdDev                Median                 IQR            Outliers     OPS            Rounds  Iterations
  --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  test_list_services_without_details       620.0175 (1.0)        748.3653 (1.0)        666.8922 (1.0)      63.0435 (1.05)       623.8694 (1.0)      108.3981 (1.07)          1;0  1.4995 (1.0)           5           1
  test_list_services_with_details        1,270.5686 (2.05)     1,403.4215 (1.88)     1,348.9908 (2.02)     60.2470 (1.0)      1,384.6504 (2.22)     100.8497 (1.0)           1;0  0.7413 (0.49)          5           1
  --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

With cache:

-------------------------------------------------------------------------------------------- benchmark: 2 tests -------------------------------------------------------------------------------------------
  Name (time in ms)                           Min                 Max                Mean             StdDev              Median                IQR            Outliers     OPS            Rounds  Iterations
  -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  test_list_services_without_details     676.1354 (1.0)      787.9862 (1.0)      716.5263 (1.0)      42.6909 (1.24)     705.7252 (1.0)      44.0494 (1.50)          1;0  1.3956 (1.0)           5           1
  test_list_services_with_details        756.4908 (1.12)     843.0671 (1.07)     782.8207 (1.09)     34.4919 (1.0)      771.2704 (1.09)     29.3169 (1.0)           1;1  1.2774 (0.92)          5           1
  -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Related issue/s

How to test

Checklist

codecov · 2022-03-08T13:22:11Z

Codecov Report

Merging #2874 (8c525ee) into master (dcad2db) will decrease coverage by 4.3%.
The diff coverage is 80.0%.

@@           Coverage Diff            @@
##           master   #2874     +/-   ##
========================================
- Coverage    79.2%   74.8%   -4.4%     
========================================
  Files         674     674             
  Lines       27620   27624      +4     
  Branches     3220    3220             
========================================
- Hits        21876   20671   -1205     
- Misses       4993    6240   +1247     
+ Partials      751     713     -38

Flag	Coverage Δ
integrationtests	`65.5% <ø> (-0.1%)`	⬇️
unittests	`70.0% <80.0%> (-4.8%)`	⬇️

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

Impacted Files	Coverage Δ
.../settings-library/src/settings_library/postgres.py	`86.8% <0.0%> (ø)`
...src/simcore_service_catalog/api/routes/services.py	`59.1% <100.0%> (+1.7%)`	⬆️
...service_webserver/projects/_project_models_rest.py	`0.0% <0.0%> (-100.0%)`	⬇️
...erver/src/simcore_service_webserver/rest_models.py	`0.0% <0.0%> (-92.7%)`	⬇️
...rc/simcore_service_webserver/computation_config.py	`0.0% <0.0%> (-88.9%)`	⬇️
...mcore_service_webserver/projects/nodes_handlers.py	`0.0% <0.0%> (-71.5%)`	⬇️
...simcore_service_webserver/director/director_api.py	`0.0% <0.0%> (-65.9%)`	⬇️
...src/simcore_service_webserver/activity/handlers.py	`28.0% <0.0%> (-63.2%)`	⬇️
...rc/simcore_service_webserver/version_control_db.py	`24.7% <0.0%> (-62.0%)`	⬇️
...service_webserver/meta_modeling_version_control.py	`27.1% <0.0%> (-60.0%)`	⬇️
... and 61 more

pcrespov

I'll wait until you merge previous PR to finish the review. For the moment, here are some inputs

pcrespov · 2022-03-08T15:37:25Z

packages/settings-library/src/settings_library/postgres.py

@@ -57,6 +62,11 @@ def dsn(self) -> str:
        )
        return dsn

+    @cached_property
+    def dsn_with_async_sqlalchemy(self) -> str:
+        dsn = self.dsn.replace("postgresql", "postgresql+asyncpg")


THOUGHT: Since we are going to deprecate aiopg this means that this will be the final dsn.
For that reason, I would directly implement it properly as

@cached_property def dsn_with_async_sqlalchemy(self) -> str: dsn: str = PostgresDsn.build( scheme="postgresql+asycnpg", user=self.POSTGRES_USER, password=self.POSTGRES_PASSWORD.get_secret_value(), host=self.POSTGRES_HOST, port=f"{self.POSTGRES_PORT}", path=f"/{self.POSTGRES_DB}", ) return dsn

You can even add a deprecation warning if using dsn ...

The replace in addition is dangerous ... for instance, what if any of the attributes, i.e. user or password ,etc is postgresql??

pcrespov · 2022-03-08T15:38:53Z

services/catalog/src/simcore_service_catalog/api/routes/services.py

@@ -65,8 +70,19 @@ def _prepare_service_details(
    return validated_service


+def _build_cache_key(fct, *args, **kwargs):


wild :-D ...

pcrespov · 2022-03-08T15:42:51Z

services/catalog/src/simcore_service_catalog/api/routes/services.py

 @router.get("", response_model=List[ServiceOut], **RESPONSE_MODEL_POLICY)
 @cancellable_request
+@cached(


let's see if i got it right: this caches the handler's return and below cached_registry_services is a completely different cache, right?

Since one wraps the other and they have the same TTL ... is really the internal making a difference?

they don't have the same TTL
actually the call to the director-v0 is always returning the same info unless the registry changed.

pcrespov · 2022-03-08T15:46:04Z

services/catalog/src/simcore_service_catalog/api/routes/services.py

-    get_registry_services_task = director_client.get("/services")
+    # caching this steps brings down the time to generate it at the expense of being sometimes a bit out of date
+    @cached(ttl=DIRECTOR_CACHING_TTL)
+    async def cached_registry_services() -> Deque[Tuple[str, str, Dict[str, Any]]]:


MINOR: the name should not incorporate cached_ since this is a decorated property (can have it or not)
Q: here you do not worry about the key-name? does this mean that different requests to this entry will reuse this cache? it so, does the access of different request might play a role here?

aiocache default key uses module name, method name and so on. here there is no key to worry about. and all users use the same one

GitHK

Nice.

GitHK · 2022-03-11T07:03:50Z

services/catalog/src/simcore_service_catalog/api/routes/services.py

@@ -39,6 +39,7 @@
 }

 DIRECTOR_CACHING_TTL = 5 * MINUTE
+LIST_SERVICES_CACHING_TTL = 30


Question, why not raise this even more? Since this just returns the list of existing services? You could cache for several minutes. It's not like services get released so fast.

Also an endpoint can be added for internal platform usage to bust the cache if needed.

it's not only new services in the registry, but you can also change the access rights, the names, thumbnail and this kind of stuff. therefore I would not extend this too much.

This reverts commit b969b8d.

sanderegg added the changelog:🎨enhancement label Mar 8, 2022

sanderegg added this to the R.Schumann+1 milestone Mar 8, 2022

sanderegg requested review from GitHK, pcrespov, odeimaiz and mguidon March 8, 2022 13:20

sanderegg self-assigned this Mar 8, 2022

sanderegg force-pushed the enhancement/cache_services_request branch from 08c91d3 to 95887ea Compare March 8, 2022 13:26

pcrespov reviewed Mar 8, 2022

View reviewed changes

sanderegg removed request for GitHK, odeimaiz and mguidon March 8, 2022 16:00

sanderegg force-pushed the enhancement/cache_services_request branch from 95887ea to cfbfb92 Compare March 8, 2022 20:20

setup caching for services request

e176ebc

sanderegg force-pushed the enhancement/cache_services_request branch from cfbfb92 to e176ebc Compare March 10, 2022 17:15

@pcrespov review: do not use replace

8c525ee

sanderegg requested review from pcrespov and GitHK March 10, 2022 17:19

pcrespov approved these changes Mar 11, 2022

View reviewed changes

GitHK approved these changes Mar 11, 2022

View reviewed changes

sanderegg merged commit b969b8d into ITISFoundation:master Mar 11, 2022

sanderegg deleted the enhancement/cache_services_request branch March 11, 2022 07:23

mrnicegyu11 pushed a commit to mrnicegyu11/osparc-simcore that referenced this pull request Mar 14, 2022

Revert "✨ Cache list service request in catalog (ITISFoundation#2874)"

8c2bd85

This reverts commit b969b8d.

sanderegg mentioned this pull request Apr 1, 2022

maintenance/scaling of the platform ITISFoundation/osparc-issues#428

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Cache list service request in catalog #2874

✨ Cache list service request in catalog #2874

sanderegg commented Mar 8, 2022 •

edited

codecov bot commented Mar 8, 2022 •

edited

pcrespov left a comment

pcrespov Mar 8, 2022

pcrespov Mar 8, 2022

pcrespov Mar 8, 2022

sanderegg Mar 10, 2022

pcrespov Mar 8, 2022

sanderegg Mar 10, 2022

GitHK left a comment

GitHK Mar 11, 2022

sanderegg Mar 11, 2022

		@@ -65,8 +70,19 @@ def _prepare_service_details(
		return validated_service


		def _build_cache_key(fct, args, *kwargs):

✨ Cache list service request in catalog #2874

✨ Cache list service request in catalog #2874

Conversation

sanderegg commented Mar 8, 2022 • edited

What do these changes do?

Related issue/s

How to test

Checklist

codecov bot commented Mar 8, 2022 • edited

Codecov Report

pcrespov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GitHK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanderegg commented Mar 8, 2022 •

edited

codecov bot commented Mar 8, 2022 •

edited