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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Oct 31, 2024
Merged

Moved run ID management to knapsack #1929

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
28b74f3
Moved run ID management to knapsack
cesarfda Oct 31, 2024
5284a22
fixing assingment
cesarfda Oct 31, 2024
d9a22ef
Refactor run ID management in knapsack to generate ID on retrieval
cesarfda Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions cmd/launcher/launcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,15 @@ func runLauncher(ctx context.Context, cancel func(), multiSlogger, systemMultiSl
flagController := flags.NewFlagController(slogger, stores[storage.AgentFlagsStore], fcOpts...)
k := knapsack.New(stores, flagController, db, multiSlogger, systemMultiSlogger)

// Generate a new run ID
newRunID := ulid.New()
// Set the run ID in knapsack
k.SetRunID(newRunID)

// Apply the run ID to both logger and slogger
logger = log.With(logger, "run_id", newRunID)
slogger = slogger.With("run_id", newRunID)

// start counting uptime
processStartTime := time.Now().UTC()

Expand Down
15 changes: 15 additions & 0 deletions ee/agent/knapsack/knapsack.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ import (
"go.etcd.io/bbolt"
)

// Package-level runID variable
var runID string

// type alias Flags, so that we can embed it inside knapsack, as `flags` and not `Flags`
type flags types.Flags

Expand Down Expand Up @@ -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) {
Copy link
Contributor

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
}

Copy link
Contributor Author

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

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor Author

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.

runID = id
k.slogger.With("run_id", id)
k.systemSlogger.With("run_id", id)
Copy link
Contributor

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.

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 did that in launcher but missed in Knapsack. I have pushed out another commit to fix this with the assignment.

}

// GetRunID retrieves the run ID from the knapsack
func (k *knapsack) GetRunID() string {
return runID
}

// Logging interface methods
func (k *knapsack) Slogger() *slog.Logger {
return k.slogger.Logger
Expand Down
9 changes: 3 additions & 6 deletions pkg/log/multislogger/multislogger.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"log/slog"
"os"

"github.com/kolide/kit/ulid"
slogmulti "github.com/samber/slog-multi"
)

Expand Down Expand Up @@ -34,17 +33,15 @@ var ctxValueKeysToAdd = []contextKey{

type MultiSlogger struct {
*slog.Logger
handlers []slog.Handler
launcherRunId string
handlers []slog.Handler
}

// New creates a new multislogger if no handlers are passed in, it will
// create a logger that discards all logs
func New(h ...slog.Handler) *MultiSlogger {
ms := &MultiSlogger{
// setting to fanout with no handlers is noop
Logger: slog.New(slogmulti.Fanout()),
launcherRunId: ulid.New(),
Logger: slog.New(slogmulti.Fanout()),
}

ms.AddHandler(h...)
Expand All @@ -69,7 +66,7 @@ func (m *MultiSlogger) AddHandler(handler ...slog.Handler) {
Pipe(slogmulti.NewHandleInlineMiddleware(utcTimeMiddleware)).
Pipe(slogmulti.NewHandleInlineMiddleware(ctxValuesMiddleWare)).
Handler(slogmulti.Fanout(m.handlers...)),
).With("launcher_run_id", m.launcherRunId)
)
}

func utcTimeMiddleware(ctx context.Context, record slog.Record, next func(context.Context, slog.Record) error) error {
Expand Down
Loading