refactor: add ability to restore context #111
Conversation
Codecov Report
@@ Coverage Diff @@
## main #111 +/- ##
============================================
- Coverage 58.37% 58.35% -0.02%
- Complexity 255 256 +1
============================================
Files 87 87
Lines 1314 1323 +9
Branches 39 39
============================================
+ Hits 767 772 +5
- Misses 517 521 +4
Partials 30 30
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -16,8 +19,12 @@ | |||
|
|
|||
| class PlatformGrpcContextBuilder implements GrpcContextBuilder { | |||
|
|
|||
| private final Cache<String, GraphQlRequestContext> contextCache = | |||
| CacheBuilder.newBuilder().expireAfterAccess(Duration.ofMinutes(1)).maximumSize(1000).build(); | |||
There was a problem hiding this comment.
I used a small short lived cache to do this as the actual transformation to grpc context is lossy - it's not possible to recreate the initial context directly, and the period of time we need to be able to restore a context is typically very small (the time it takes to build the request to the time it takes to make the request).
There was a problem hiding this comment.
Any chance the max size will be hit?
There was a problem hiding this comment.
I guess it could (this would be > 1000 requests per minute), but that's not actually an issue since guava caches are LRU and it'll just help us evict entries quicker (we only need them for a short order of time, typically on the order of millis). The issue would be if we have to handle more than the cache size (1000) concurrently which is far beyond the current scale. If it makes you feel better I can make it configurable :)
There was a problem hiding this comment.
Ok. So it's rare a cache item stays in there for more than a couple of milliseconds?
There was a problem hiding this comment.
It's rare that we need it to, but the only mechanisms that remove it currently are the size and time expirations.
The expected flow is that when making a remote request in a DAO class, an impl will trade their GQL context for a GRPC context. The GRPC context is used to make the request. The need for this class is for any hooks at the GRPC level - if we're instrumenting or intercepting the calls, we can get the GRPC context that was used to make them, but we can't (without this PR) get the GQL context that was used to create that GRPC context. If we want to make other calls into GQL code in those interceptor classes, we need that GQL context back (so the expected elapsed time here is from initiating to intercepting a client request - at least for my use case)
Description
Added the ability to restore the graphql request context from the grpc context. This is required for grpc instrumentation and interceptors which do not give direct access to the graphql context.
Testing
Updated unit tests
Checklist: