-
Notifications
You must be signed in to change notification settings - Fork 103
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
added new store for uptime history and uptime tracking with knapsack #1923
Conversation
cmd/launcher/launcher.go
Outdated
// start counting uptime | ||
processStartTime := time.Now() | ||
|
||
k.UpTimeHistoryStore().Set([]byte("process_start_time"), []byte(processStartTime.Format(time.RFC3339))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HistoryStore
makes me think this would hold a series of process start times - but I feel like we would want to do that eventually anyway so probably not worth changing the name. might be nice to add a comment somewhere (maybe here or near knapsack definition) saying that we only currently track the running process start time, but would like to push these into a historical list in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to LauncherHistoryStore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question about naming -- LGTM!
ee/agent/storage/stores.go
Outdated
@@ -18,6 +18,7 @@ const ( | |||
ServerProvidedDataStore Store = "server_provided_data" // The store used for pushing values from server-backed tables. | |||
TokenStore Store = "token_store" // The store used for holding bearer auth tokens, e.g. the ones used to authenticate with the observability ingest server. | |||
ControlServerActionsStore Store = "action_store" // The store used for storing actions sent by control server. | |||
UpTimeHistoryStore Store = "uptime_history" // The store used for storing uptime history. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we'd want something like LauncherHistoryStore Store = "launcher_history"
-- I think that might be a little more extensible for the future, and would match osquery_instance_history
. I don't feel strongly about it, though -- what do others think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that more too, it makes sense to me to report as just uptime
for the table column but internally here it is a little more confusing what this refers to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, I updated the name to LauncherHistoryStore
pkg/osquery/table/launcher_info.go
Outdated
uptime := "" | ||
|
||
if uptimeBytes != nil { | ||
startTime, _ := time.Parse(time.RFC3339, string(uptimeBytes)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should probably check the error here before processing startTime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pkg/osquery/table/launcher_info.go
Outdated
if err != nil { | ||
// No logger here, so we can't easily log. Move on with blank values | ||
publicKey = "" | ||
fingerprint = "" | ||
} | ||
|
||
uptimeBytes, _ := upTimeHistoryStore.Get([]byte("process_start_time")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should probably check the error here before processing uptimeBytes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I avoided returning the err because I don't want uptime blocking the rest of the info on the table, so I set a default value to use if the uptime err are not nil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great! just a few small comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
@@ -33,6 +33,7 @@ func MakeStores(ctx context.Context, slogger *slog.Logger, db *bbolt.DB) (map[st | |||
storage.ServerProvidedDataStore, | |||
storage.TokenStore, | |||
storage.ControlServerActionsStore, | |||
storage.LauncherHistoryStore, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow. Looking at the stores here, I'm not sure there was an obvious one. But creating a new one to hold a single value feels big. Done now though
Resolves #1702
Preview: