Add Interceptor Methods for Skip and Update Rendering #1297
Conversation
steve-the-edwards
requested review from
zach-klippenstein and
a team
as code owners
steve-the-edwards
force-pushed
the
sedwards/extend-interceptor
branch
from
9f2152b to
8f6d19e
Compare
| * as well as two extra parameters: | ||
| * This interface's methods mirror the methods of [StatefulWorkflow], and three additional methods, | ||
| * explained below. | ||
| * Each method returns the same thing as the corresponding method on [StatefulWorkflow], and\ |
| @@ -129,6 +142,21 @@ public interface WorkflowInterceptor { | |||
| session: WorkflowSession | |||
| ): Snapshot? = proceed(state) | |||
|
|
|||
| /** | |||
| * Called when a [RenderingAndSnapshot] is updated from the runtime (i.e. when the [StateFlow] | |||
| * is provided a new value - this may still be de-duplicated). | |||
There was a problem hiding this comment.
"this may still be de-duplicated" => by consumers of the state flow, based on some other things?
There was a problem hiding this comment.
by the StateFlow itself which de-duplicates emissions downstream.
| /** | ||
| * Called when the rendering is skipped because no state has changed after action cascade(s). | ||
| */ | ||
| public fun onRenderingSkipped(): Unit = Unit |
There was a problem hiding this comment.
if the outcome of an action loop is always either "onRenderingSkipped" or "onRenderingUpdated", then I would actually merge those and pass in a sealed class of "Updated" vs "Skipped", which would make this behavior a lot clearer.
There was a problem hiding this comment.
ya I think that sounds reasonable!
| * | ||
| * @return [RenderingAndSnapshot]: a possibly modified [RenderingAndSnapshot]. | ||
| */ | ||
| public fun <R> onRenderingUpdated( |
There was a problem hiding this comment.
It's not clear from the name that this only applies to the rendering published out to the external consumer. Combined with Py's suggestion below, could we name it something like "onExternalRenderingEvent" (with an updated/skipped parameter).
There was a problem hiding this comment.
yes I can rename this.
| * 1. [onRenderingSkipped] - called when the rendering is short-circuited because no state | ||
| * change was detected as part of the action cascade(s) and the | ||
| * [RuntimeConfig.RENDER_ONLY_WHEN_STATE_CHANGES] is enabled. | ||
| * 1. [onRenderingUpdated] - called when a [RenderingAndSnapshot] is passed to the [StateFlow] | ||
| * returned by [renderWorkflowIn]. It is passed the [RenderingAndSnapshot] which could be | ||
| * decorated. |
There was a problem hiding this comment.
This feels a bit awkward to me. Up to this point, everything in WorkflowInterceptor is 1-1 with every individual workflow in the system. These new methods, IIUC, apply to the runtime as a whole. I feel like it violates SRP? I don't want to be overly pedantic about it and I don't have any real practical reason to not put them here.
I do wonder if this would be a good excuse to figure out a dedicated, higher-level tracing API though like we'd need for compose-based workflows. I can jump on that this week a bit probably, although I'll be leaving on Friday for 2 weeks.
There was a problem hiding this comment.
onSessionStarted actually existed before these changes (I just moved the documentation and made it clear).
Yes, agreed that we are violating SRP and there is an opening here for a higher/orthogonal tracing dedicated API. I also don't want to tackle that with this work if possible and the interceptor can handle this without growing any more unwieldy than it already is, I think.
steve-the-edwards
force-pushed
the
sedwards/extend-interceptor
branch
from
8f6d19e to
248cc99
Compare
steve-the-edwards
changed the base branch from
sedwards/drain-exclusive-actions
to
main
| public data object RenderPassSkipped : RuntimeLoopOutcome | ||
| public data class RenderPassesComplete<R>( | ||
| val renderingAndSnapshot: RenderingAndSnapshot<R> | ||
| ) : RuntimeLoopOutcome |
There was a problem hiding this comment.
We've avoided data class and data object in public types.
There was a problem hiding this comment.
Thanks for catching that. Will fix.
| * 1. [onRenderingSkipped] - called when the rendering is short-circuited because no state | ||
| * change was detected as part of the action cascade(s) and the | ||
| * [RuntimeConfig.RENDER_ONLY_WHEN_STATE_CHANGES] is enabled. | ||
| * 1. [onRenderingUpdated] - called when a [RenderingAndSnapshot] is passed to the [StateFlow] | ||
| * returned by [renderWorkflowIn]. It is passed the [RenderingAndSnapshot] which could be | ||
| * decorated. |
There was a problem hiding this comment.
Stale kdoc, there is only one new method now.
There was a problem hiding this comment.
Will update.
| * Called when a the runtime loop has "iterated." Typically this the application of 1 action | ||
| * and 1 render pass. |
There was a problem hiding this comment.
| * Called when a the runtime loop has "iterated." Typically this the application of 1 action | |
| * and 1 render pass. | |
| * Called to report the [outcome] of each tick of the runtime loop. In the simplest case | |
| * this is the application of one action and one render pass, but optimizations can | |
| * change that: |
| * If there is no state change with the `RENDER_ONLY_WHEN_STATE_CHANGES` optimization, | ||
| * it might not be a render pass at all, in which case [RenderPassSkipped] is the outcome. | ||
| * In the case of the `CONFLATE_STALE_RENDERINGS` optimization it could be multiple render | ||
| * passes. | ||
| * If there is at least one render pass, then [RenderPassesComplete] is passed as the outcome, | ||
| * which includes the actual [RenderingAndSnapshot] returned from the runtime. |
There was a problem hiding this comment.
Maybe make this a bullet list.
steve-the-edwards
force-pushed
the
sedwards/extend-interceptor
branch
from
248cc99 to
6286ac8
Compare
This is necessary in order to do effective tracing with optimizations enabled.
These are also helpful and quite logical methods for the interceptor, though they are driven from the runtime rather than the workflow definition (so eventually could be extracted to their own type of interceptor interface)