Conversation
|
…mentation cleanup.
There was a problem hiding this comment.
Pull request overview
Adds the new Cachet family of crates (tier abstractions, service integration, and a moka-backed memory tier) and wires shared test helpers into the workspace.
Changes:
- Introduce new crates:
cachet,cachet_tier,cachet_memory,cachet_service(+ docs, examples, benches, and tests). - Add
testing_aidshelpers for log/metric assertions and updateseatbelttests to use them. - Reduce
uniflight::Merger::executefuture size by boxing the user future.
Reviewed changes
Copilot reviewed 87 out of 88 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/uniflight/src/lib.rs | Box user future in execute. |
| crates/testing_aids/src/metrics.rs | Add OTel metric test helper. |
| crates/testing_aids/src/log.rs | Add log capture helper. |
| crates/testing_aids/src/lib.rs | Re-export new test helpers. |
| crates/testing_aids/Cargo.toml | Add OTel deps for testing. |
| crates/seatbelt/src/timeout/service.rs | Switch tests to testing_aids. |
| crates/seatbelt/src/testing.rs | Remove local testing helpers. |
| crates/seatbelt/src/retry/service.rs | Switch tests to testing_aids. |
| crates/seatbelt/src/fallback/service.rs | Switch tests to testing_aids. |
| crates/seatbelt/src/breaker/service.rs | Switch tests to testing_aids. |
| crates/seatbelt/src/breaker/engine/engine_telemetry.rs | Switch tests to testing_aids. |
| crates/seatbelt/Cargo.toml | Add testing_aids dependency. |
| crates/cachet_tier/tests/tier.rs | Integration tests for CacheTier. |
| crates/cachet_tier/tests/entry.rs | Integration tests for CacheEntry. |
| crates/cachet_tier/src/tier.rs | Define core CacheTier trait. |
| crates/cachet_tier/src/testing.rs | Add MockCache test tier. |
| crates/cachet_tier/src/lib.rs | Crate entry / exports. |
| crates/cachet_tier/src/error.rs | Cache error type + helpers. |
| crates/cachet_tier/src/entry.rs | Cache entry value + TTL metadata. |
| crates/cachet_tier/src/dynamic.rs | Type-erased DynamicCache. |
| crates/cachet_tier/logo.png | Crate asset (LFS). |
| crates/cachet_tier/favicon.ico | Crate asset (LFS). |
| crates/cachet_tier/README.md | Crate documentation. |
| crates/cachet_tier/Cargo.toml | New crate manifest/features. |
| crates/cachet_tier/CHANGELOG.md | New changelog stub. |
| crates/cachet_service/tests/adapter.rs | Adapter integration tests. |
| crates/cachet_service/src/request.rs | Define service request/response types. |
| crates/cachet_service/src/lib.rs | Crate entry / exports. |
| crates/cachet_service/src/ext.rs | CacheServiceExt helpers. |
| crates/cachet_service/src/adapter.rs | ServiceAdapter implementation. |
| crates/cachet_service/logo.png | Crate asset (LFS). |
| crates/cachet_service/favicon.ico | Crate asset (LFS). |
| crates/cachet_service/README.md | Crate documentation. |
| crates/cachet_service/Cargo.toml | New crate manifest. |
| crates/cachet_service/CHANGELOG.md | New changelog stub. |
| crates/cachet_memory/tests/cache.rs | In-memory tier integration tests. |
| crates/cachet_memory/src/tier.rs | Moka-backed InMemoryCache tier. |
| crates/cachet_memory/src/lib.rs | Crate entry / exports. |
| crates/cachet_memory/src/builder.rs | InMemoryCacheBuilder API. |
| crates/cachet_memory/logo.png | Crate asset (LFS). |
| crates/cachet_memory/favicon.ico | Crate asset (LFS). |
| crates/cachet_memory/README.md | Crate documentation. |
| crates/cachet_memory/Cargo.toml | New crate manifest. |
| crates/cachet_memory/CHANGELOG.md | New changelog stub. |
| crates/cachet/tests/wrapper.rs | Cache wrapper integration tests. |
| crates/cachet/tests/future_size.rs | Future size regression tests. |
| crates/cachet/tests/builder.rs | Builder integration tests. |
| crates/cachet/src/wrapper.rs | Telemetry + TTL wrapper tier. |
| crates/cachet/src/telemetry/mod.rs | Telemetry module wiring. |
| crates/cachet/src/telemetry/metrics.rs | OTel meter/instrument builders. |
| crates/cachet/src/telemetry/ext.rs | Clock timing helper future. |
| crates/cachet/src/telemetry/config.rs | Telemetry configuration builder. |
| crates/cachet/src/telemetry/cache.rs | Telemetry recording logic + tests. |
| crates/cachet/src/telemetry/attributes.rs | Telemetry attribute constants. |
| crates/cachet/src/refresh.rs | Background refresh (TTR). |
| crates/cachet/src/lib.rs | Crate entry / feature exports. |
| crates/cachet/logo.png | Crate asset (LFS). |
| crates/cachet/favicon.ico | Crate asset (LFS). |
| crates/cachet/examples/wrapped.rs | Telemetry/TTL example. |
| crates/cachet/examples/stampede_protection.rs | Stampede protection example. |
| crates/cachet/examples/simple.rs | Basic cache usage example. |
| crates/cachet/examples/service_middleware_composition.rs | Middleware composition example. |
| crates/cachet/examples/service_as_storage.rs | Service-as-storage example. |
| crates/cachet/examples/refresh.rs | Refresh (TTR) example. |
| crates/cachet/examples/multi_tier_service.rs | Multi-tier + service example. |
| crates/cachet/examples/multi_tier.rs | Multi-tier promotion policy example. |
| crates/cachet/examples/mock_testing.rs | MockCache testing example. |
| crates/cachet/examples/get_or_insert.rs | Loading helper example. |
| crates/cachet/examples/fallback.rs | Fallback tier example. |
| crates/cachet/examples/error_handling.rs | Typed error handling example. |
| crates/cachet/examples/dynamic.rs | Dynamic cache type-erasure example. |
| crates/cachet/examples/cache_as_service.rs | Cache-as-service example. |
| crates/cachet/benches/refresh.rs | Refresh overhead benchmark. |
| crates/cachet/benches/operations.rs | Core ops + wrapper benchmark. |
| crates/cachet/benches/dynamic.rs | Dynamic dispatch benchmark. |
| crates/cachet/README.md | Crate documentation. |
| crates/cachet/Cargo.toml | New crate manifest/features. |
| crates/cachet/CHANGELOG.md | New changelog stub. |
| README.md | Add cachet + TOC tweaks. |
| Cargo.toml | Add new workspace crate deps. |
| CHANGELOG.md | Link cachet changelog. |
| .spelling | Add new terminology. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// Returns the captured log output as a string. | ||
| /// | ||
| /// # Panics | ||
| /// | ||
| /// Panics if the captured bytes are not valid UTF-8, which should not happen since `tracing_subscriber` writes UTF-8. | ||
| #[must_use] | ||
| pub fn output(&self) -> String { | ||
| String::from_utf8_lossy(&self.buffer.lock().unwrap()).to_string() | ||
| } |
There was a problem hiding this comment.
The output() docs claim it panics on invalid UTF-8, but the implementation uses String::from_utf8_lossy, which never panics. Please either switch to String::from_utf8(...).expect(...) if you want a panic, or update the docs to match the current behavior.
| /// Thread-local log capture buffer for testing. | ||
| /// | ||
| /// Uses `tracing_subscriber::fmt::MakeWriter` to capture formatted log output | ||
| /// into a thread-local buffer that can be inspected in tests. | ||
| #[derive(Debug, Clone, Default)] | ||
| pub struct LogCapture { | ||
| buffer: std::sync::Arc<Mutex<Vec<u8>>>, | ||
| } |
There was a problem hiding this comment.
LogCapture is described as "thread-local", but it stores logs in an Arc<Mutex<Vec<u8>>> and clones share the same buffer across threads. Either make the storage truly thread-local (e.g., thread_local!) or update the docs to describe it as a shared capture buffer used with thread-local subscriber installation.
| // Box the future immediately to keep state machine size small. | ||
| // Without boxing, the entire Fut type would be embedded in our state machine. | ||
| // With boxing, we only store a 16-byte pointer. | ||
| let boxed = Box::pin(func()); |
There was a problem hiding this comment.
This comment says boxing stores a "16-byte pointer", but Box::pin(func()) here produces a Pin<Box<Fut>>, which is a thin pointer (typically 8 bytes on 64-bit). Consider correcting the comment to avoid misleading future maintainers (the broader point about reducing state machine size still holds).
| use crate::breaker::{EngineFake, HealthInfo, Stats}; | ||
| use crate::metrics::{create_meter, create_resilience_event_counter}; | ||
| use crate::testing::MetricTester; | ||
| use testing_aids::MetricTester; |
There was a problem hiding this comment.
Could you move all changes to non-cachet-related files to a stand-alone PR?
| //! .memory() | ||
| //! .build(); | ||
| //! | ||
| //! cache.insert(&"key".to_string(), CacheEntry::new(42)).await?; |
There was a problem hiding this comment.
Shouldn't this just be:
| //! cache.insert(&"key".to_string(), CacheEntry::new(42)).await?; | |
| //! cache.insert("key", CacheEntry::new(42)).await?; |
| //! # if cfg!(miri) { return; } // moka is incompatible with Miri | ||
| //! # futures::executor::block_on(async { | ||
| //! | ||
| //! let clock = Clock::new_frozen(); |
There was a problem hiding this comment.
@martintmk I really wish this had a different name, like new_fake. I find new_frozen confusing at best...
@schgoo Might it be better to show a runtime API example, rather than a test one? So like new_tokio?
| #![cfg_attr(coverage_nightly, feature(coverage_attribute))] | ||
| #![cfg_attr(docsrs, feature(doc_cfg))] | ||
|
|
||
| //! Flexible multi-tier caching with telemetry and TTL support. |
There was a problem hiding this comment.
I think you want a much bigger intro here to sell why this is a good idea. Why multi-level caching, why background refresh, what flexibility does the crate provide, why use this instead of other caches, what is cache stampede and how is it avoided, what are the major types exposed by this crate, how do cache tiers compose and interact, what are the companion crates (the other cachet crates), etc.
There was a problem hiding this comment.
Also, you should be describing all the cargo features
| //! | ||
| //! **Levels:** DEBUG (hit/miss/ok), INFO (expired/inserted/invalidated/refresh), ERROR (error) | ||
|
|
||
| pub mod builder; |
There was a problem hiding this comment.
I think you should be making all the modules private and surface the few types you have in this crate all at the top as cachet::*
| } | ||
| } | ||
|
|
||
| /// Constructor and access methods. |
There was a problem hiding this comment.
I don't think it's common to have a doc comment on top of an impl block. It shows up in a wierd place in the generated doc. And in this case, it's actually misleading to the user since the constructor isn't public.
I'd remove all comments on impl blocks througout the PR.
| /// # }); | ||
| /// ``` | ||
| #[derive(Debug)] | ||
| pub struct Cache<K, V, S = ()> { |
There was a problem hiding this comment.
Why "S"? It's a CacheTier trait, so how about CT instead?
| /// | ||
| /// This erases the concrete storage type, allowing caches with different | ||
| /// storage implementations to be used interchangeably at runtime. | ||
| #[must_use] |
There was a problem hiding this comment.
Explain the downsides. Why isn't every cache dynanically diapatched?
There was a problem hiding this comment.
Why isn't this just a builder parameter? Are there scenarios where you have a cache for a while, and then decide to make it dynamic?
There was a problem hiding this comment.
Originally, obviously the difference was that the non-dynamic cache would not require any heap allocations, especially for futures. That has somewhat changed, because stack-allocating the cache futures resulted in huge future sizes (32KB+, bounded on the number of layers), as the Rust type system had to fight with all the possible states whene everything is wrapped in Result<Option<...>>, etc. The solution to this was to Box futures in the fallback tier to limit future sizes to size(L1 future) + size(pointer). At this point, there is a small benefit to not having a dynamic cache, in the event that you either:
- Expect a high hit % on your L1 cache, which is stack-allocated, or
- Your cache has no nesting to begin with.
So, since the argument that we should be using dynamic caches at all times has changed since I first added the dynamic cache, it's worth revisiting. Should we even allow non-dynamic caches?
There was a problem hiding this comment.
Depends on the perf delta I think.
Could we pick a heuristic and figure this out ourselves? If the cache is a single tier, we don't do dynmic, otherwise we do? Or something along those lines?
| /// `.stampede_protection()`) | ||
| /// - Clock management for time-based operations | ||
| /// | ||
| /// This type does NOT implement `CacheTier` - it is always the outermost wrapper. |
There was a problem hiding this comment.
Explain dynamic vs. non-dynamic caches.
| /// # Ok::<(), cachet::Error>(()) | ||
| /// # }); | ||
| /// ``` | ||
| pub async fn get(&self, key: &K) -> Result<Option<CacheEntry<V>>, Error> { |
There was a problem hiding this comment.
Can we support Borrow semantics here so you can use a &str to lookup a cache that has String as key?
| /// # Errors | ||
| /// | ||
| /// Returns an error if the underlying cache tier operation fails. | ||
| pub async fn invalidate(&self, key: &K) -> Result<(), Error> { |
There was a problem hiding this comment.
"remove" would probsbly be a better more normal name here.
I very much worry about the fact this is fallible. If I use a cache and need to remove stale data, like a revoked token, and the removal from cache fails, what am I supposed to do as an application? I think my only recourse would be to restart. Or I suppose I could maintain a separate in-memory cache of things that should have been removed but haven't, which seems like something this cache could handle internally. If you've ever had removal failures coming in, then record those failures and use them to prevent returning bad data to the caller. The TTL on this 'failed removals' cache just needs to be longer than the TTL of the original data and it should be reliable.
There was a problem hiding this comment.
I called it invalidate because I was using the Moka cache API as inspiration. I think there they were capturing the idea that invalidate doesn't return the removed entry, whereas remove does.
I agree it would be problematic for invalidate to fail but making it infallible might just mean that errors get swallowed in the cache implementation. At least this way, a user will know if it failed for some reason. You make a good point though that dealing with that failure might not be simple. Is your idea that we would make this method infallible at the call level, but fallible at the CacheTier level? Otherwise, we would have no way of knowing if the operation failed.
There was a problem hiding this comment.
I think you leave it with the same fallible signature, but provide some feedback:
"When this returns an error, it is signaling that some cache tier has failed to remove the cache entry. You may wish to retry the call using normal retry semantics in order to attempt the removal again later. However, even when an error is returned, this particular cache instance will return NotFound for the supplied cache key for the duration of the TTL. Since cache tiers can be shared across process boundaries (like when a tier is Redis), a failure to remove an entry means that the entry msy remain visible to other processes."
Or something like that...
|
|
||
| /// Returns the number of entries in the cache, if supported by the underlying storage. | ||
| #[must_use] | ||
| pub fn len(&self) -> Option<u64> { |
There was a problem hiding this comment.
How does this interact with TTL?
When you return a count, can it include items which have already exceeded their TTL? If the TTL is checked primarily when someone calls get on a key, or TTL is checked periodically (like every minute), then len may return innacurate counts.
There was a problem hiding this comment.
Yes, it would be up to the CacheTier implementation to actually remove items that have exceeded their TTL, just as it's up to them to implement len. Potentially, we could fill this gap by adding another method on CacheTier to notify when an item is expired? Or we could just call invalidate on it.
There was a problem hiding this comment.
I was really asking to document this imprecision to clarify to the user that this length value is a mere approximation of reality and doesn't take into account potentially evicted items. It is in fact a high-water mark more than an accurate count of the valid items in the cache.
Maybe this function deserves a different name?
| /// | ||
| /// Like [`get_or_insert`](Self::get_or_insert), but the provided function can fail. | ||
| /// Only successful results are cached - errors are not cached, allowing retries. | ||
| /// |
There was a problem hiding this comment.
Do you support any form of negative caching? That is, caching the fact a particular key is simply invalid and doesn't need to be tried against the SOT again.
There was a problem hiding this comment.
Yes, you can create a custom PromotionPolicy with a predicate match against your type V
There was a problem hiding this comment.
Maybe advertise this as a key feature of the cache in your high-level overview, and show an example of this.
In particular, I would expect negative caching to happen when the callback in this try_xxx function returns an error. "DB said no permission" so cache that fact for me. How would the failure in the callback result in negative caching happening?
| # Licensed under the MIT License. | ||
|
|
||
| [package] | ||
| name = "cachet_memory" |
There was a problem hiding this comment.
Maybe this crate should be called cachet_moka?
I hope we can design our own cache which is more scalable than moka. So it would be good not to 'hog' the name cachet_memory.
There was a problem hiding this comment.
Well, I was thinking we could just replace Moka with the new cache internally here without changing the API. Do you think that's unreasonable?
There was a problem hiding this comment.
Sure, that's reasonable since the API is not very specialized. Then maybe stop mentioning "moka" in the docs? If it's not a guarantee of the design and it just an implementation detail, let's just hide that fact.
| //! - **Thread-safe**: Safe for concurrent access from multiple tasks | ||
| //! - **Zero external types**: Builder API avoids exposing moka in your public API | ||
|
|
||
| pub mod builder; |
There was a problem hiding this comment.
Same as the other crate, don't make the modules public.
| /// ``` | ||
| #[derive(Debug)] | ||
| pub struct Cache<K, V, S = ()> { | ||
| pub(crate) name: CacheName, |
There was a problem hiding this comment.
Shouldn't we make the hasher configurable? There is a substantial perf difference between different hashers in the Rust world. The default hasher used by HashMap is quite slow, being optimized for protection against key attacks. Most caches don't need this protection and so could run considerably faster by using an alternate hashing algorithm.
| allowed_external_types = ["cachet_tier::tier::CacheTier", "thread_aware::core::ThreadAware"] | ||
|
|
||
| [dependencies] | ||
| cachet_tier = { path = "../cachet_tier" } |
There was a problem hiding this comment.
All three dependencies here should be using workspace = true
| cachet_memory = { path = "../cachet_memory", optional = true } | ||
| cachet_service = { path = "../cachet_service", optional = true } | ||
| cachet_tier = { path = "../cachet_tier" } |
There was a problem hiding this comment.
| cachet_memory = { path = "../cachet_memory", optional = true } | |
| cachet_service = { path = "../cachet_service", optional = true } | |
| cachet_tier = { path = "../cachet_tier" } | |
| cachet_memory = { workspace = true, optional = true } | |
| cachet_service = { workspace = true, optional = true } | |
| cachet_tier.workspace = true |
|
|
||
| [dev-dependencies] | ||
| alloc_tracker = { workspace = true } | ||
| cachet_tier = { path = "../cachet_tier", features = ["test-util"] } |
There was a problem hiding this comment.
| cachet_tier = { path = "../cachet_tier", features = ["test-util"] } | |
| cachet_tier = { workspace = true, features = ["test-util"] } |
| opentelemetry_sdk = { workspace = true, features = ["logs", "testing"] } | ||
| recoverable = { workspace = true } | ||
| seatbelt = { workspace = true, features = ["retry", "tower-service"] } | ||
| testing_aids = { path = "../testing_aids" } |
There was a problem hiding this comment.
| testing_aids = { path = "../testing_aids" } | |
| testing_aids.workspace = true |
| ] | ||
|
|
||
| [dependencies] | ||
| cachet_tier = { path = "../cachet_tier" } |
There was a problem hiding this comment.
| cachet_tier = { path = "../cachet_tier" } | |
| cachet_tier.workspace = true |
| tokio = { workspace = true, features = [ | ||
| "macros", | ||
| "rt", | ||
| ], default-features = false } |
There was a problem hiding this comment.
default-feature is already set by the workspace-level Cargo.toml
Cachet (pronounced: ka-SHAY)
Meaning: Prestige, distinction, or high status that makes something admired or desirable.
Examples:
“The brand has a certain cachet in tech circles.”
“Winning that award gave the company international cachet.”
It’s about reputation and perceived quality.
Origin: French, meaning “seal” or “stamp of approval.”