-
Notifications
You must be signed in to change notification settings - Fork 10
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
feat(reports): reports sidecar, configurable caching #104
Conversation
Hi @andrewazores! Add at least one of the required labels to this PR Required labels are : chore,ci,cleanup,docs,feat,fix,perf,refactor,style,test |
47563b9
to
2503918
Compare
f111b6a
to
089ed5d
Compare
This PR/issue depends on: |
089ed5d
to
9065078
Compare
/build_test |
Workflow started at 1/8/2024, 9:45:42 AM. View Actions Run. |
CI build and push: All tests pass ✅ |
1 similar comment
CI build and push: All tests pass ✅ |
bc724fe
to
a7f7642
Compare
/build_test |
Workflow started at 1/9/2024, 10:28:45 AM. View Actions Run. |
CI build and push: All tests pass ✅ |
CI build and push: At least one test failed ❌ |
Guess I broke something. I'll figure it out. |
/build_test |
Workflow started at 1/9/2024, 1:49:21 PM. View Actions Run. |
CI build and push: All tests pass ✅ |
1 similar comment
CI build and push: All tests pass ✅ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
This reverts commit c381e6a.
The merge-base changed after approval.
08783da
to
d596144
Compare
/build_test |
Workflow started at 1/9/2024, 3:27:18 PM. View Actions Run. |
CI build and push: All tests pass ✅ |
1 similar comment
CI build and push: All tests pass ✅ |
Replaces #65
Based on #62
Depends on #62
Depends on cryostatio/cryostat-core#301
cryostat-reports
instance, configurable insmoketest.bash
with the new-r
flag, andReports sourced from active recordings are never stored in S3-backed cache, because active recordings are assumed to be too dynamic and rapidly-changing to make sense to cache this way. They are cached in-memory only and for a short duration by default simply to reduce the cost of clients rapidly requesting report generations for the same recording.
Reports sourced from archived recordings are sourced from the in-memory cache if available. If not, they are sourced from the S3-backed cache if available. If not, then they are freshly generated, then stored in S3, then stored in memory, and returned to the client. Since archived reports are (by definition) static data, the in-memory cache duration for them is longer by default to improve performance while also (hopefully) not collecting too many and exerting undue memory pressure on the Cryostat server. If an archived recording report is requested after it has been evicted from in-memory cache then it will be retrieved from S3 rather than regenerated, since this should also be a much cheaper operation. This S3-backed scheme also means that archived reports have a lifecycle decoupled from the Cryostat server, so the Cryostat server can be restarted and the cached reports for archived recordings can still be retrieved and used to repopulate in-memory caches as needed. Here is a log snippet illustrating the tiered caching action for an archived report and a sidecar report generator:
Here is an illustration of Cryostat retrieving a cached report from S3 after it has timed out and been evicted from the in-memory cache:
The above parameters are configurable using the following new properties:
TODO: after rebasing, the in-memory active recording cache doesn't seem to expire on a timer, it just always returns cached entries.TODO: the
MemoryCachingReportsListener
doesn't currently work as expected. When it observes Target losses, the Targets always appear to have no associated active recordings, so the listener cannot determine which entries to invalidate. This is a relatively minor issue since the cache entries will expire after a relatively short duration anyway, and each cache entry is simply the report JSON document, which is pretty small. I think this is actually a deeper issue to do with how the relation between the Target, ActiveRecording, and DiscoveryNode entities works, not specific to this PR - it's just made apparent by this cache invalidation listener.As in previous Cryostat releases, if there is no sidecar report generator configured, then the Cryostat server itself will handle report generation requests.