Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bbolt abstraction for statistic tracking #1659

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

zackattack01
Copy link
Contributor

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 a SizeBytes int64 and GetStats 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.)

Copy link
Contributor

@directionless directionless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I read this correct, I think:

  1. there's a new statsstore thing
  2. which has a GetStates method that returns []byte I assume this is intended for use in a table/flare/etc.
  3. (It's really the old logic from agent.GetStats)
  4. This is wired into knapsack, passed in via the initialization, and returned bare with StorageStatTracker()

This feels a little weird to me. I'm not sure I have a simple suggestion though, I think it's highlighting something awkward that already existed. But it feels like this logic is spanning a bunch of places. And I wonder:

  • 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)
  • 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

I'm curious if you have more thoughts

Size int64
}

type Stats struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
type Stats struct {
type stats struct {

I think? Or maybe even make it anonymous in the GetStats method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i'll get that cleaned up, ty!

@zackattack01
Copy link
Contributor Author

@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:

  • all of the 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 bucket
  • The checkup info will include bucket level statistics, but there is also a notion of global bbolt db statistics - keeping it with Stores 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.
    • while there are ways to do this so the code looks clean, I could not find ways that I would want to debug in a year 😀
    • none of the functionality here seems critical path so I really tried to avoid overcomplicating the nice clean 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

@directionless
Copy link
Contributor

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

@zackattack01 zackattack01 marked this pull request as draft March 22, 2024 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants