[server] SpiceDB: Add request-level caching based on AsyncLocalStorage+ZedTokens #18893
Conversation
| export function setZedTokenToContext(zedToken: StoredZedToken) { | ||
| const ctx = asyncLocalStorage.getStore(); | ||
| if (ctx) { | ||
| ctx.zedToken = zedToken; |
There was a problem hiding this comment.
does it really work? I thought the only way to modify the storage is to use run?
There was a problem hiding this comment.
My understanding was that this will work when called from code that runs inside runWithContext. As we run all API calls inside, I thought it should.
Might be wrong, though. 🙃
/cc @svenefftinge
There was a problem hiding this comment.
just checking, I don't know whether you can mutate, or only read
There was a problem hiding this comment.
Yes, it would work. Just not super cool to hijack the "log"-context here. Do we want to move the code that sets the asyncLocalStorage out to something more general and make use of that from logging?
There was a problem hiding this comment.
Just not super cool to hijack the "log"-context here. Do we want to move the code that sets the asyncLocalStorage out to something more general and make use of that from logging?
Totally agree. Would it be ok to merge this now (so we get some data), and me working on this in parallel?
| * Works as a caching decorator for SpiceDBAuthorizerImpl. Delegates the actual caching strategy to ZedTokenCache. | ||
| */ | ||
| @injectable() | ||
| export class CachingSpiceDBAuthorizer implements SpiceDBAuthorizer { |
There was a problem hiding this comment.
would we use the cache directly in the original implmentation?
Using a delegation pattern introduces an abstraction that we don't really need but adds additional complexity with the inversify bindings but also requires to expose the token bit on the API level.
There was a problem hiding this comment.
As discussed in sync: I wanted to focus on getting results first, before fine-tuning the implementation.
That being said, I agree on the inversify bindings and exposing impl details in the interface. 💯 Not so much on the "decorator is complexity" but oh well 🙃
Here is a branch where addressed all the review comments from this PR, given that the underlying idea works out: #18898
| } | ||
|
|
||
| async readRelationships(req: v1.ReadRelationshipsRequest): Promise<v1.ReadRelationshipsResponse[]> { | ||
| // pass through with given consistency/caching for now |
There was a problem hiding this comment.
would be good to also use the token here. Is that somehow different or more complicated than in other cases?
There was a problem hiding this comment.
No. But as the only use of the API is for introspection, I thought we always want fullyConsistent. 👍
| * To make this work we make the "assumption" that ZedTokens string (meant to be opaque) represent a timestamp which we can order. This is at least true for the MySQL datastore we are using. | ||
| */ | ||
| @injectable() | ||
| export class RequestLocalZedTokenCache implements ZedTokenCache { |
There was a problem hiding this comment.
Could the functions in here all be sync?
There was a problem hiding this comment.
Do we need an interface for this?
|
|
||
| export interface StoredZedToken { | ||
| token: string; | ||
| timestamp: number; |
There was a problem hiding this comment.
why store both elements?
There was a problem hiding this comment.
Performance considerations: I don't want to always decode/encode the cached token (in fact, I did not even implement encode).
| export type ZedTokenCacheKV = [objectRef: v1.ObjectReference | undefined, token: string | undefined]; | ||
| export const ZedTokenCache = Symbol("ZedTokenCache"); | ||
| export interface ZedTokenCache { | ||
| get(objectRef: v1.ObjectReference): Promise<string | undefined>; |
There was a problem hiding this comment.
I understood we would ignore the resources entirely with this simple approach, but just globally store a single token that we use when it exists
| } | ||
| } | ||
|
|
||
| async consistency(resourceRef: v1.ObjectReference | undefined): Promise<v1.Consistency> { |
There was a problem hiding this comment.
this functionality could be the responsibility of the spicedb-authorizer not the cache itself.
I believe the cache really only needs get and update. Both being sync and very simple, update would only update if the given token is higher than the current one.
| export function setZedTokenToContext(zedToken: StoredZedToken) { | ||
| const ctx = asyncLocalStorage.getStore(); | ||
| if (ctx) { | ||
| ctx.zedToken = zedToken; |
There was a problem hiding this comment.
Yes, it would work. Just not super cool to hijack the "log"-context here. Do we want to move the code that sets the asyncLocalStorage out to something more general and make use of that from logging?
|
Server currently logs |
geropl
force-pushed
the
gpl/738-zedtokens-3
branch
from
6a92654 to
088cb73
Compare
geropl
force-pushed
the
gpl/738-zedtokens-3
branch
from
88dc424 to
e96f061
Compare
|
@svenefftinge The preview works now. It was this line I missed in the Go implementation... I ended up generating SpiceDB internal proto implementations for |
|
/unhold |
Description
This is a first attempt to address EXP-738, to come up with the least-invasive caching strategy for SpiceDB using ZedTokens:
In parallel, we are looking to visualize the cache metrics SpiceDB emits. ✔️
This PR also comes with two tests. To make them work with the context-based caching, all relevant methods are executed with
runWithContextto simulate API calls.Follow-up cleanup PR: #18898
Summary generated by Copilot
🤖 Generated by Copilot at 6a92654
Refactor and cache the SpiceDB authorizer in the server component. Use ZedTokens to ensure consistency and avoid stale reads and writes. Simplify the Authorizer class methods. Add unit tests for the caching logic.
Related Issue(s)
Part of EXP-738
How to test
Documentation
Preview status
Gitpod was successfully deployed to your preview environment.
Build Options
Build
Run the build with werft instead of GHA
Run Leeway with
--dont-testPublish
Installer
Add desired feature flags to the end of the line above, space separated
Preview Environment / Integration Tests
If enabled this will build
install/previewIf enabled this will create the environment on GCE infra
Valid options are
all,workspace,webapp,ide,jetbrains,vscode,ssh. If enabled,with-previewandwith-large-vmwill be enabled./hold