feat(cogs): ChangeStream trait, preparation for backend instrumentation - #590
feat(cogs): ChangeStream trait, preparation for backend instrumentation#590matt-codecov wants to merge 2 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## matth/storage-inventory-tracker #590 +/- ##
===================================================================
- Coverage 88.15% 87.97% -0.18%
===================================================================
Files 101 104 +3
Lines 16526 16776 +250
===================================================================
+ Hits 14568 14759 +191
- Misses 1958 2017 +59
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| /// a change stream for our storage backends. | ||
| /// | ||
| /// See [module docs](self). | ||
| pub trait InventorySink: fmt::Debug + Send + Sync + 'static { |
There was a problem hiding this comment.
Calling out that this interface is close to what is being created in #582. I don't have a concrete design in mind yet, but it seems the "inventory" concept would be capable of solving both problems:
- Storage cost tracking, which requires to know which files are stored until when
- GC / eviction, which requires to know which files are stored until when
There was a problem hiding this comment.
Apart from that we don't want to couple the two in-flight PRs too much, so this is probably more relevant for the other PR:
The biggest difference is how they are needed: Cost tracking requires the inventory independent of the backend, and will export it via kafka to a remote system. Conversely, the S3 and FS backends need the inventory as part of their inner workings. Per design, GC is part of the backend's responsibility.
We might be able to separate this cleanly if we:
- Hook the inventory up in the service like the PR here does (sort of like an event bus)
- Allow multiple inventory exporters
- One is the kafka sink used for cost tracking
- Another one is a database sink (like sqlite, PG) for GC, configured/hooked when S3/FS are used
- GC is still an external command then, that queries said database
There was a problem hiding this comment.
my direction here is very much "fail open" and errors are logged and swallowed. if inventory tracking is load-bearing for GC in some backends, that'll need to change
correct me if i'm wrong: for self-hosted, in order to support the automatic TTI/TTL GC that we advertise, we have to implement it ourselves atop filesystem and S3 as we anticipate those will be popular in self-hosted deployments?
There was a problem hiding this comment.
That's right, and that's what @aldy505's PR linked in my initial comment does. The trait is called Keeper there and it's intended to be integrated directly into the backends. Conceptually, they hold similar responsibilities.
While for cogs accounting errors are less critical, we should still find an implementation that's correct by design or has well-understood failure modes. Failing loudly/openly does work, but we need to look closely at what happens during concurrent requests.
94e1e3a to
f819003
Compare
matt-codecov
changed the title
4fd0bdc to
560244f
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks, the documentation and tests made this easy to review.
There is a good foundation for separation of concerns, but the current config mixes them again:
ChangeStreamtracks logical changes to objects agnostic of how this information is interpreted. This is what backends are instrumented with, and it can be hooked in various ways.InventoryTrackeris the component that maps this to cost-tracking semantics.
There are several downsides: ChangeStream's config now needs to know about the possible implementations of inventory tracker, comes with an optional kafka config, there could now be different change streams per backend, it needs to know about shared resource IDs, and about which scopes are important for cost tracking. This makes it harder to use the change stream for GC.
This can be solved:
- Keep
ChangeStreamtrait tied to objectstore-service concepts like it is now. - Instantiate ChangeStream globally as backends are being created, and pass it generically into the backends. This removes the need for an internal factory.
- Have every backend annotate its stream items with an identifier. Initially that can be hard coded, but that's the one thing we can make overridable in backend config (i.e.
gcs1vsgcs2). - The config for which ChangeStream to instantiate should live adjacent to the backends. There's only a single one available now, the inventory tracker, and then we directly surface the inventory tracker's possible implementations there.
- Inventory tracker and its config is the level at which we
- map the built-in backend IDs to
shared_resource_id(cost concern) - extract org ID and project ID from scopes (if available)
- optionally map use case to app features (currently part of the usecase config)
- map the built-in backend IDs to
| gcp_auth = { workspace = true } | ||
| humantime = { workspace = true } | ||
| humantime-serde = { workspace = true } | ||
| objectstore-inventory-tracker = { workspace = true, features = ["kafka"] } |
There was a problem hiding this comment.
Let's make this optional = true and enable it with a dedicated feature flag. That way, we can control for individual builds, whether we have kafka-based inventory tracking included or not. For sandbox cross builds and for self-hosted releases in the future, we'll not include this.
This also means that we do not have to add cmake to the cross-build Dockerfile anymore.
| fn scope_id(id: &ObjectId, scope: &str) -> Option<u64> { | ||
| id.scopes().get_value(scope)?.parse().ok() |
There was a problem hiding this comment.
Does usage-accountant require these to be numeric? Especially at this level we should not make assumptions about the type and expose org and project as strings, and let the consumer handle this.
If this is a requirement, let's do the conversion at the innermost level possible (in the kafka implementation, likely).
There was a problem hiding this comment.
usage-accountant doesn't have proj/org IDs
the new kafka topic (which i just merged and released, oops) has them as integers. not too late to change, would just need a new PR and release
| /// for the record format. | ||
| /// | ||
| /// Logs, counts, and swallows errors returned by the [`InventoryTracker`]. | ||
| pub struct KafkaStream { |
There was a problem hiding this comment.
Let's rename this type. Technically, this can be any kind of InventoryTracker, it purely depends on which producer is used to instantiate it.
It just so happens that build_producer and thus the change stream factory currently only have a kafka field; see my overall review comment for this.
| /// [`ListenerConfig`], so connections and the send queue are not duplicated per backend. | ||
| #[derive(Debug, Clone, Deserialize, Serialize)] | ||
| #[serde(default)] | ||
| pub struct SinkConfig { |
There was a problem hiding this comment.
This is a complete clone of KafkaConfig. We should probably make this an enum that allows for different kinds of InventoryTracker producers, one of which is the kafka variant. That way, the interface/config becomes extensible.
Even better: Don't assume any of that and just pass a complete InventoryTracker into the constructor of this type. That way, the concerns are split entirely.
560244f to
a2576e8
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a2576e8. Configure here.
| && apt-get update -qq \ | ||
| && apt-get upgrade -y \ | ||
| && apt-get install -y --no-install-recommends make protobuf-compiler libprotobuf-dev pkg-config git libssl-dev:amd64 gcc-x86-64-linux-gnu g++-x86-64-linux-gnu \ | ||
| && apt-get install -y --no-install-recommends make protobuf-compiler libprotobuf-dev pkg-config git libssl-dev:amd64 libcurl4-openssl-dev:amd64 gcc-x86-64-linux-gnu g++-x86-64-linux-gnu \ |
There was a problem hiding this comment.
Unnecessary Kafka build dependencies
Low Severity
libcurl4-openssl-dev was added for librdkafka, but storage-cogs stays off in these builds: the cross image builds objectstore without that feature, release builds only pass profiling, and test-python runs plain cargo build. objectstore-server also does not forward storage-cogs. The optional feature exists specifically so sandbox/cross and default builds avoid Kafka native deps, so these installs do not match that design.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit a2576e8. Configure here.
3 participants
Depends on #588
ChangeStreamtrait wrapsInventoryTrackerto log/count/swallow errors and allow aNoopStreamto be plugged in for tests to avoid pulling in Kafka stuff.