-
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
Moved run ID management to knapsack #1929
Conversation
ee/agent/knapsack/knapsack.go
Outdated
k.slogger.With("run_id", id) | ||
k.systemSlogger.With("run_id", id) |
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.
Do we need e.g. --
k.slogger = k.slogger.With("run_id", id)
k.systemSlogger = k.systemSlogger.With("run_id", id)
? I think .With
creates and then modifies a clone, so it wouldn't update k.slogger.
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.
Yeah, I did that in launcher but missed in Knapsack. I have pushed out another commit to fix this with the assignment.
ee/agent/knapsack/knapsack.go
Outdated
@@ -59,6 +62,18 @@ func New(stores map[storage.Store]types.KVStore, flags types.Flags, db *bbolt.DB | |||
return k | |||
} | |||
|
|||
// SetRunID sets the run ID in the knapsack | |||
func (k *knapsack) SetRunID(id string) { |
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.
Do you need this method?
I'd probably do something like:
func (k *knapsack) GetRunID() string {
if runID == "" {
runID = ulid.New()
}
return runID
}
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 think that wouldn't cover the scenario where we want every time runLauncher is called to have a newID, I assume we could run into a situation where the ID is persisted if runLauncher is called after an update. Let me know if that's not the case and I can change this
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.
Chatted on slack -- I think this is worth testing. I'm not sure if the restart/exec would have access to the same memory
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 think if we did need to create a specific create method, my inclination would be to make k.NewRunID()
and call it on startup. I think passing in the string feels wrong.
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 was able to test the scenario and after the update we do get a new ID when we generate the ID in the GET method. I refactored the code to not have a set.
// Generate a new run ID | ||
newRunID := k.GetRunID() |
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 know this has merged, and it's fine, but I'd probably skip the variable here, and put k.GetRunID()
into the two With
functions
k.slogger.Logger = k.slogger.Logger.With("run_id", runID) | ||
k.systemSlogger.Logger = k.systemSlogger.Logger.With("run_id", runID) |
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.
Hrm. This here, and the part in runLauncher
feels off somehow.
This pull request moves the run ID system to improve logging and tracking across the application. The changes include generating a unique run ID, setting it in the knapsack, and applying it to the logger and slogger for consistent tracking.
Resolves #1706
Run ID Implementation:
cmd/launcher/launcher.go
: Added code to generate a new run ID and set it in the knapsack, logger, and slogger.ee/agent/knapsack/knapsack.go
: Introduced a package-levelrunID
variable and addedGetRunID
method to manage the run ID within the knapsack. [1] [2]Logger Updates:
pkg/log/multislogger/multislogger.go
: Removed thelauncherRunId
variable from theMultiSlogger
struct and its initialization, and updated theAddHandler
method to no longer include thelauncher_run_id
. [1] [2] [3] [4]