Add logging around Zarr manifest cache interactions #170
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #170 +/- ##
==========================================
- Coverage 46.97% 46.33% -0.64%
==========================================
Files 27 29 +2
Lines 3930 3984 +54
==========================================
Hits 1846 1846
- Misses 2084 2138 +54 ☔ View full report in Codecov by Sentry. |
jwodder
force-pushed
the
cache-log
branch
2 times, most recently
from
ec2eadd
to
57b47dd
Compare
jwodder
force-pushed
the
cache-log
branch
4 times, most recently
from
60afdd3
to
35f169c
Compare
jwodder
force-pushed
the
cache-log
branch
2 times, most recently
from
a168770
to
ade0877
Compare
jwodder
force-pushed
the
cache-log
branch
2 times, most recently
from
58eeb5e
to
30936a2
Compare
|
@yarikoptic Ready for review. |
|
@yarikoptic Ping. |
README.md
Outdated
| - `-Z <INT>`, `--zarrman-cache-bytes <INT>` — Specify the maximum number of | ||
| bytes of parsed Zarr manifest files to store in the Zarr manifest cache at | ||
| once [default: 100 MiB] |
There was a problem hiding this comment.
may be just worth making it
| - `-Z <INT>`, `--zarrman-cache-bytes <INT>` — Specify the maximum number of | |
| bytes of parsed Zarr manifest files to store in the Zarr manifest cache at | |
| once [default: 100 MiB] | |
| - `-Z <INT>`, `--zarrman-cache-megabytes <INT>` — Specify the maximum number of | |
| megabytes of parsed Zarr manifest files to store in the Zarr manifest cache at | |
| once [default: 100] |
since unlikely we would ever look into less than 1 MiB? That might make it easier to use this option
| }) | ||
| .time_to_idle(MANIFEST_CACHE_IDLE_EXPIRY) | ||
| .eviction_listener(|path, manifest, cause| { | ||
| tracing::debug!( |
There was a problem hiding this comment.
test coverage annotation is funny here but suggests that most here is not tested Does it mean that logging on eviction doesn't happen ie. evictions do not happen during testing? (I am not yet a good reader of Rust, sorry)
There was a problem hiding this comment.
None of the caching is currently tested.
| start: MissMoment | ||
| end: MissMoment | ||
| #: Number of other misses in progress as of the end of this miss | ||
| parallel: int |
There was a problem hiding this comment.
interesting why of interest "as of the end" and not "as of the beginning"?
There was a problem hiding this comment.
It all comes out the same either way, but I had to pick one.
| for k, v in misses_in_progress.items() | ||
| ) | ||
| misses_in_progress = {} | ||
| last_accesses = {} |
There was a problem hiding this comment.
is this the end and there should be no other hits or a new dandidav starts and we should get new separate stats or you are just collating them across all runs?
There was a problem hiding this comment.
This is the start of a new dandidav process, so any information specific to the previous process needs to be cleared.
| 1 for ts in last_accesses.values() if ts > last_access | ||
| ) | ||
| else: | ||
| recency_index = len(last_accesses) |
There was a problem hiding this comment.
when/how do we hit this on "hit"?... not exactly following
There was a problem hiding this comment.
This happens if the provided logs start in the middle of a server process and a hit is encountered for an entry that was previously accessed before the start of the logs.
In case we ever add logs with different types of events or for other caches
|
@yarikoptic Please re-review; can this be merged now? |
|
Yes, thank you! Let's try it out in action! |
This PR makes the following changes:
-Z/--zarrman-cache-bytesoption for setting the cache size (default: 100 MiB)tools/for taking one or more Heroku log files and producing statistics about the Zarr manifest cachePart of #118.