Add new CacheStrategy & CacheMiddleware classes #3

jblz · 2024-08-20T13:51:01Z

Add POST to the request methods that we can cache, but default to not caching them
Add a method for queries to declare a TTL via get_cache_ttl method
Add debug logging to fetch & cache
Add mechanism to bypass cache via remote_data_blocks_bypass_cache filter

This isn't perfect yet, but it gives us a jumping-off point for fine-tuning our caching strategy.

Cold Cache	Warm Cache

jblz · 2024-08-21T17:26:53Z

inc/http-client/class-cache-strategy.php

+	}
+
+	protected function should_bypass_cache( RequestInterface $request ) {
+		$bypass = $request->getHeader( self::HEADER_BYPASS_CACHE );


We can inspect the request here to e.g. put tighter scrutiny on POST requests.

e.g. We discussed inspecting the GraphQL operation for mutations and setting those to not cache.

Additionally, we discussed using a method on e.g. the QueryContext that we leverage to bypass the cache.

mhsdef · 2024-08-21T20:12:47Z

inc/http-client/class-cache-strategy.php

+use Psr\Http\Message\RequestInterface;
+use RemoteDataBlocks\Logging\LoggerManager;
+
+class CacheStrategy extends GreedyCacheStrategy {


Worth s/CacheStrategy/CustomCacheStrategy?

Just thinking, in other context, like when instantiated, some sort of qualifier like that might convey more meaning. And RemoteDataBlocksCacheStrategy feels a tad long even for me (though I'm open to it!).

I think the PHP namespace will suffice for now to indicate these are bespoke & not from the composer lib. If / when we have more than one implementation of the strategy / middleware classes, that's probably a good time to be more specific about the naming, IMO.

I ended up prefixing these with Rdb -- feel free to push a change if anyone has a preference.

inc/http-client/class-cache-middleware.php

mhsdef

LGTM, didn't test.

It would be nice to have unit tests on the cache strategy class. Maybe take Cursor for a spin on that if you haven't tried it yet? I suspect it'd do nicely with a class that's so direct as that one.

mehmoodak · 2024-08-22T08:53:46Z

inc/http-client/class-cache-strategy.php

+		$uri_string = $uri->getScheme() . '://' . $uri->getHost() . $uri->getPath();
+		$method     = $request->getMethod();
+		$body       = $request->getBody()->getContents();
+		return $method . ' ' . $uri_string . ' ' . $body;


Is it safe to log the content of the $body?

Is it safe to log the content of the $body?

In production, we'd not want to do this for sure. Since we're just logging to the debug log, it's probably not all that critical. At least for now as we debug the middleware and strategy.

mehmoodak · 2024-08-22T08:55:24Z

inc/http-client/class-cache-strategy.php

+		$logger = LoggerManager::instance();
+
+		if ( $this->should_bypass_cache( $request ) ) {
+			$logger->debug( 'Bypassing cache: ' . self::getRequestString( $request ) );


Should we have any prefix in debugging messages which will make the search easier?

Just pass a string to LoggerManager::instance(), it accepts a namespace

@chriszarate Seems like the Logger constructor / create fn does, but not the static LoggerManager::instance fn:

remote-data-blocks/inc/logging/logger-manager/logger-manager.php

Lines 27 to 33 in 01eaa59

public static function instance(): Logger {

if ( null === self::$instance ) {

self::$instance = Logger::create( self::$log_namespace );

}

return self::$instance;

}

...unless I'm missing something?

Any reason to not just use the Logger::create fn here?

Oh, right. I guess we already have a namespace and don't really need one? Or we want multi-level namespaces?

I think @mehmoodak's suggestion was for multi-level namespaces to be able to more-easily identify that the debug log came from e.g. the HTTP Client module (similar to how the debug npm package allows multi-level prefixing / filtering.

We could drill in via Query Monitor like this:

I think I'll file it to an issue for an enhancement outside this PR.

mehmoodak · 2024-08-22T08:56:45Z

+1 on @mhsdef suggestion of testing.

jblz

+1 to tests for these. I wanted to make sure folks agreed w/ the direction before writing them. I'll see how feasible it is.

jblz · 2024-08-22T14:16:33Z

inc/http-client/class-cache-strategy.php

+use Psr\Http\Message\RequestInterface;
+use RemoteDataBlocks\Logging\LoggerManager;
+
+class CacheStrategy extends GreedyCacheStrategy {


I think the PHP namespace will suffice for now to indicate these are bespoke & not from the composer lib. If / when we have more than one implementation of the strategy / middleware classes, that's probably a good time to be more specific about the naming, IMO.

jblz · 2024-08-22T14:18:12Z

inc/http-client/class-cache-strategy.php

+		$uri_string = $uri->getScheme() . '://' . $uri->getHost() . $uri->getPath();
+		$method     = $request->getMethod();
+		$body       = $request->getBody()->getContents();
+		return $method . ' ' . $uri_string . ' ' . $body;


Is it safe to log the content of the $body?

In production, we'd not want to do this for sure. Since we're just logging to the debug log, it's probably not all that critical. At least for now as we debug the middleware and strategy.

jblz · 2024-08-22T15:56:07Z

inc/http-client/class-cache-strategy.php

+	}
+
+	protected function should_bypass_cache( RequestInterface $request ) {
+		$bypass = $request->getHeader( self::HEADER_BYPASS_CACHE );


e.g. We discussed inspecting the GraphQL operation for mutations and setting those to not cache.

Additionally, we discussed using a method on e.g. the QueryContext that we leverage to bypass the cache.

jblz · 2024-08-22T15:56:16Z

inc/http-client/class-cache-strategy.php

+		$logger = LoggerManager::instance();
+
+		if ( $this->should_bypass_cache( $request ) ) {
+			$logger->debug( 'Bypassing cache: ' . self::getRequestString( $request ) );


with debug logging

…s well as GET

Refactors & fixes to make tests pass

… add/guzzle-caching-strategy

composer.json

chriszarate · 2024-08-27T22:12:10Z

tests/inc/http-client/HttpClientTest.php

@@ -11,6 +11,8 @@
 use GuzzleHttp\Exception\ConnectException;
 use GuzzleHttp\Psr7\Request;
 use GuzzleHttp\Exception\RequestException;
+use Kevinrob\GuzzleCache\Storage\VolatileRuntimeStorage;


this Kevinrob person has some good adjectives, A++

…arser

… add/guzzle-caching-strategy

jblz · 2024-09-25T15:46:16Z

inc/HttpClient/RdbCacheMiddleware.php

+	/**
+	 * @var array<string, bool>
+	 */
+	// phpcs:ignore WordPress.NamingConventions.ValidVariableName.PropertyNotSnakeCase, SlevomatCodingStandard.TypeHints.PropertyTypeHint.MissingNativeTypeHint


These are needed to comply with the parent class' declaration

jblz · 2024-09-25T15:55:56Z

@chriszarate want to give this another once over?

inc/Config/QueryRunner/QueryRunner.php

chriszarate

Looks great, just had one suggestion (to improve my own code) :)

…trategy

… add/guzzle-caching-strategy

…emote-data-blocks into add/guzzle-caching-strategy

jblz force-pushed the add/guzzle-caching-strategy branch 2 times, most recently from addcd13 to 22d148b Compare August 21, 2024 13:29

jblz marked this pull request as ready for review August 21, 2024 13:36

jblz requested review from mhsdef, maxschmeling, stoph, mehmoodak and shekharnwagh August 21, 2024 13:36

jblz changed the title ~~Add new CacheStrategy class~~ Add new CacheStrategy & CacheMiddleware classes Aug 21, 2024

jblz commented Aug 21, 2024

View reviewed changes

mhsdef reviewed Aug 21, 2024

View reviewed changes

inc/http-client/class-cache-middleware.php Outdated Show resolved Hide resolved

mhsdef approved these changes Aug 21, 2024

View reviewed changes

mehmoodak reviewed Aug 22, 2024

View reviewed changes

jblz commented Aug 22, 2024

View reviewed changes

jblz added 6 commits August 27, 2024 17:33

Add new CacheStrategy class

93f469c

with debug logging

customize our TTL header

84aa0f8

Implement our own CacheMiddleware class that supports POST requests a…

683ab17

…s well as GET

add logging on fetch & cache; add mechanism to bypass cache

3470574

do not cache graphql mutations

ac27360

Add phpunit test cases to the HttpClientTest

8ae5f3b

Refactors & fixes to make tests pass

jblz force-pushed the add/guzzle-caching-strategy branch from 5495e0a to 8ae5f3b Compare August 27, 2024 21:38

jblz requested a review from mhsdef August 27, 2024 21:38

Merge branch 'trunk' of github.com:Automattic/remote-data-blocks into…

6bd4714

… add/guzzle-caching-strategy

chriszarate reviewed Aug 27, 2024

View reviewed changes

composer.json Outdated Show resolved Hide resolved

chriszarate reviewed Aug 27, 2024

View reviewed changes

jblz and others added 3 commits August 28, 2024 09:23

Use much smaller Graphql Parsing lib: infinityloop-dev/graphpinator-p…

32a1c9b

…arser

POC cache TTL injection

917bde9

Add GraphQLMutationContext

9aba65a

chriszarate and others added 11 commits August 28, 2024 14:59

Provide sensible defaults for most queries

838c205

Remove GraphQL request body inspection

0717874

Merge branch 'trunk' of github.com:Automattic/remote-data-blocks into…

4e3956e

… add/guzzle-caching-strategy

Merge branch 'trunk' of github.com:Automattic/remote-data-blocks into…

1132feb

… add/guzzle-caching-strategy

fixup spacing

d4616ae

Merge branch 'trunk' of github.com:Automattic/remote-data-blocks into…

31e42ba

… add/guzzle-caching-strategy

fix casing on Config dir

65db305

lint fixes and type annotations / enhancements

ae0fe37

more php lint fixes

83c1439

fix type issue caused by linter

aaf6be2

Merge branch 'trunk' of github.com:Automattic/remote-data-blocks into…

8aadec8

… add/guzzle-caching-strategy

jblz commented Sep 25, 2024

View reviewed changes

jblz requested a review from chriszarate September 25, 2024 15:55

chriszarate reviewed Sep 25, 2024

View reviewed changes

inc/Config/QueryRunner/QueryRunner.php Outdated Show resolved Hide resolved

Merge branch 'trunk' into add/guzzle-caching-strategy

90932a4

chriszarate approved these changes Sep 25, 2024

View reviewed changes

chriszarate and others added 8 commits September 25, 2024 17:08

Merge remote-tracking branch 'origin/trunk' into add/guzzle-caching-s…

6cb94c0

…trategy

Fix psalm issue

cb1ef74

Merge remote-tracking branch 'origin/trunk' into add/guzzle-caching-s…

cb058da

…trategy

Merge branch 'trunk' into add/guzzle-caching-strategy

917fc00

Merge branch 'trunk' of github.com:Automattic/remote-data-blocks into…

2c6d343

… add/guzzle-caching-strategy

make get_cache_ttl return a nullable int

d148a41

Merge branch 'add/guzzle-caching-strategy' of github.com:Automattic/r…

30b0e00

…emote-data-blocks into add/guzzle-caching-strategy

tweak the constant name

757c6d7

jblz merged commit cb3fb16 into trunk Sep 26, 2024
10 checks passed

jblz deleted the add/guzzle-caching-strategy branch September 26, 2024 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new CacheStrategy & CacheMiddleware classes #3

Add new CacheStrategy & CacheMiddleware classes #3

jblz commented Aug 20, 2024 •

edited

Loading

jblz Aug 21, 2024

jblz Aug 22, 2024

mhsdef Aug 21, 2024 •

edited

Loading

jblz Aug 22, 2024

jblz Aug 28, 2024

mhsdef left a comment •

edited

Loading

mehmoodak Aug 22, 2024

jblz Aug 22, 2024

mehmoodak Aug 22, 2024

jblz Aug 22, 2024

chriszarate Aug 23, 2024

jblz Aug 23, 2024 •

edited

Loading

jblz Aug 23, 2024

chriszarate Aug 23, 2024

jblz Aug 23, 2024 •

edited

Loading

jblz Aug 23, 2024

mehmoodak commented Aug 22, 2024

jblz left a comment

jblz Aug 22, 2024

jblz Aug 22, 2024

jblz Aug 22, 2024

jblz Aug 22, 2024

chriszarate Aug 27, 2024

jblz Sep 25, 2024

jblz commented Sep 25, 2024

chriszarate left a comment

	public static function instance(): Logger {
	if ( null === self::$instance ) {
	self::$instance = Logger::create( self::$log_namespace );
	}

	return self::$instance;
	}

Add new CacheStrategy & CacheMiddleware classes #3

Add new CacheStrategy & CacheMiddleware classes #3

Conversation

jblz commented Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsdef Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhsdef left a comment • edited Loading

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

jblz Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jblz Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehmoodak commented Aug 22, 2024

jblz 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

jblz commented Sep 25, 2024

chriszarate left a comment

Choose a reason for hiding this comment

jblz commented Aug 20, 2024 •

edited

Loading

mhsdef Aug 21, 2024 •

edited

Loading

mhsdef left a comment •

edited

Loading

jblz Aug 23, 2024 •

edited

Loading

jblz Aug 23, 2024 •

edited

Loading