Separate workflow/context with different loggers

kbrock commented 2 months ago

Goal

The logging is global to all of floe. This means we do not associate certain logging information with each user/run.

Setting up a per execution log allows us to isolate the log per running instance and display it to the end user.

Implementation

The Context is a per execution concept. It contains the user's information (in execution), the user's credentials, and the runtime context. (including credentials in context has conflated the db context and the runtime context)

Since we want a per execution logger, I added logger to the Context.

Alternative

The Workflow currently contains the states and the runtime Context. The suggestion was to add the logger to the Workflow. Unfortunately, this does not work because while the various components have access to the Context, they do not have access to the Workflow.

So I propose we stay the course and use Context#logger

kbrock commented 2 months ago

update:

rebased to handle changes to cli

think this is still part of discussions

Fryguy commented 2 months ago

Overall I like the concept, but I think of context as more of the "serializable" part of the workflow, so it feels like the wrong place to put the logger. I think workflow itself feels like the more appropriate place. @agrare @kbrock thoughts?

kbrock commented 2 months ago

@Fryguy Yes, @agrare feels the same way as you do.

source.asl -- source code.
AST -- source code loaded into memory. This can be the same across multiple executions.
Execution Environment
- state input Context#state.input / Context#input
- credentials Context#credentials (recent change)
- execution history Context#state_history
- logging (was suggesting Context.logger but up to you)

I was pushing that Workflow would only be the AST, but looks like we have linked context in there. I did try and pull context out, but it didn't work so well.

Maybe we want a different object to group context (really only need the current state), credentials, logger

kbrock commented 2 months ago

update:

rebase (conflict)

kbrock commented 2 months ago

update:

fix code climate (make Cli#run smaller)

update:

fix refactor amuck.

kbrock commented 2 months ago

update:

rebase master

agrare commented 2 months ago

Other than the logger belonging to Context I think this looks good, I would have thought logger lived with a Workflow

kbrock commented 2 months ago

WIP: going to find another home for the logger. Possibly pass in a RuntimeContext, similar to an ImageProcessor context with Context State, credentials, and logger

kbrock commented 1 month ago

un-WIP

Logistically, Context#logger works much better than Workflow#logger. The latter caused too many extra variables to get passed around, specifically in the Runner.

Philosophically, I feel the Context holds all execution and user specific state, while the Workflow holds no state and is valid across executions. By this definition, the logger belongs with the Context.

Elephant: Wihle I don't like the fact that Workflow currently has a reference to Context, I'm waiting for wait to settle down before suggesting any further change there.