feat(spooler): Add capacity check for the envelope buffer

[iambriccardo]

This PR adds the capability for checking the capacity of the envelope buffer. Such capacity is computed differently based on the strategy in use:

• Disk: the disk usage is computed asynchronously and periodically updated in the background.
• Memory: the memory usage is checked via the MemoryChecker.

Closes: https://github.com/getsentry/team-ingest/issues/517, https://github.com/getsentry/team-ingest/issues/521

[iambriccardo]

Renamed to StacksManager to make it more meaningful w.r.t. the mansions of the trait implementations.

[jjbayer]

Manager in a type name is usually a red flag for unclear separation of concerns. In this case it creates new stacks parametrized with the underlying storage backend, and queries the storage backend for information. Maybe we should call this StorageBackend, and / or unite it with the other EnvelopeStore?

[iambriccardo]

The thing is that for me they are two separate entities, it's a coincidence that some of them forward the methods.

The dependency is:

flowchart TD
	StacksManager --> EnvelopeStack --> EnvelopeStore

Loading

[jjbayer]

StacksManager holds a reference to EnvelopeStore, right? So there is already a coupling between the manager and the store:

classDiagram
StacksManager "1" o-- "1" EnvelopeStore
StacksManager --> "creates" EnvelopeStack
EnvelopeStack "n" o-- "1" EnvelopeStore

Loading

IMO this can be simplified to

classDiagram
EnvelopeStack o-- "uses" EnvelopeStore
EnvelopeStore --> "creates" EnvelopeStack

Loading

(store creates stack, stack uses store)

[iambriccardo]

I feel like having the store interact with the stack is not ideal, for me the store is just offering data to some consumers, the stack is dealing with the data in a domain-specific way and the stack manager deals with creating stacks.

[jjbayer]

If we don't expect capacity to ever have more than two unit variants, I would remove this and give the trait a has_capacity method instead. That makes the intent more clear IMO.

[jjbayer]

Manager in a type name is usually a red flag for unclear separation of concerns. In this case it creates new stacks parametrized with the underlying storage backend, and queries the storage backend for information. Maybe we should call this StorageBackend, and / or unite it with the other EnvelopeStore?

[jjbayer]

See comment about await, rest are nits.

[jjbayer]

On second review, I think we should make this method sync, because we should avoid blocking the request handler on disk read / writes under all circumstances. One option to make it sync would be to change GuardedEnvelopeBuffer::has_capacity to a try_lock, and cache the resulting bool:

        match self.inner.try_lock() {
            Ok(guard) => {
                let c = guard.backend.has_capacity();
                self.cached_capacity.store(c);
                c
            },
            Err(_) => self.cached_capacity.load()
        }

[iambriccardo]

Yeah, this could be an idea, given the intrinsic delay of memory and disk usage calculation, adding one by caching it should not be a big deal. My main worry with this is that we can't guarantee updated values in any way if lock contention is arbitrarily high.

[jjbayer]

Sounds acceptable to me. We can emit a counter metric to track how often we emit cached vs fresh capacity. Once we transform this to a service, the cached capacity will be updated differently, anyway.

[iambriccardo]

Yep, I added the metric because I am interested.

[jjbayer]

nit: As discussed, this does not need to be a trait. It might be beneficial to remove it, to avoid reader's expectations like "surely there is also a MemoryEnvelopeStore".