Open zackattack01 opened 3 months ago
@directionless
If I read this correct, I think...
This is all correct, the stats are currently used in slightly different ways across 2 flare checkups and in a table
Is it cleaner to keep passing the DB to knapsack, and puts the GetStats implementation there? (Probably not, knapsack tends to be a simple wrapper)
I came to the same conclusion here- it seemed like an unnecessarily complex bit of logic to push up there and makes it just as easy to abuse usage going forward
What would it look like if GetStats was part of the stores interface, they already have the db there. This feels good, but also that it might be a little awkward if we have multiple databases
This was my instinct too (and initial approach). Things got messy for a couple reasons:
KVStore
abstractions so far are easily swappable with our current in memory implementation. For either the bbolt or in memory implementations, the implementer cares about a single bucketStores
ended up meaning we needed one store that acted as a virtual "no-bucket" store just to read global statistics without breaking all existing patterns.
Stores
Note that my opinion might be different if we decided that we could do away with the global stats, and instead just ask each store for GetStats
- This logic assumes that the kolide_launcher_db_info
should not be changed at all
I think if this all feels too complex to both of us, we should explore some other ways to get at it.
My hunch is that we should expose it via knapsack. But I agree with you about all the ways it's messy
This is the final bit of work towards removing the direct bbolt DB connection references from knapsack. The remaining references are a bit different than the prior abstraction work so far - these are all held to pull stats from bbolt. This is done at a global (not bucket) level for the most part, so utilizing the same
KVStore
pattern here did not make sense.Instead, this replaces the direct
db
connection embedded in knapsack with a new interface type:StorageStatTracker
. The interface is intentionally vague (requires only that implementers can return aSizeBytes
int64 andGetStats
json blob, because all current utilizations were tightly coupled to the bbolt logic itself. For now this seemed like the cleanest way to get where we want without overcomplicating potential future work (e.g. replacing bbolt usage in tests, changing storage methodology, etc.)