Upstreaming Runtime Monitoring and Tracing

[steve-the-edwards]

This is the evolution of a class we've long used in production internally. This takes a lot of the bookkeeping that we had there and makes it available to other pluggable WorkflowRuntimeTracers.

There is also an update to the RuntimeUpdate type that is clearer about when the runtime loop is finished (regardless of if the rendering is skipped or not).

First commit removes the old TracingWorkflowInterceptor and the corresponding trace-encoder module.

The second commit adds the WorkflowRuntimeMonitor as well as a bunch of reflection-less logging utilities that we've used for some time.

[rjrjr]

When thinking about upstreaming this I always felt bad about how com.squareup.com is hard coded into wfStripSquarePackage(). Wonder if we should expose a static property or something to allow customization. I don't think that should derail this initial commit, just food for thought.

But in the meantime, it does seem sketchy to have a default implementation that relies on a non-public function.

Is this call redundant here anyway? Do we also make downstream calls?

[steve-the-edwards]

we can remove the default.

[rjrjr]

Wise, it's a terrible default anyway. Shouldn't be making it so easy to use toString() by accident, defeats the purpose.

[steve-the-edwards]

Wonder if we should expose a static property or something to allow customization. I don't think that should derail this initial commit, just food for thought.

I thought about this too, but its a bit confusing to add and is a lot of power to munge the logs that i landed on just keeping it simple.

[rjrjr]

Do we have a unit test that will break if we screw that up?

[steve-the-edwards]

No. but we should. I will add.

[steve-the-edwards]

Did one better and just made it a constant in Worker.kt shared in both places

[rjrjr]

Making this function non-public defeats a major use case. We want feature code implementing Loggable to be able to call this on members so that the opt-in scheme works recursively. I'd be inclined to most or all of this file public -- making Loggable public without this invites serious footguns.

[steve-the-edwards]

I can make it public, but my general idea was that you would implement your own getLogString() to handle more cases than what we can include here.

[rjrjr]

Better to err on the side of privacy probably. Easier to open up later.

[steve-the-edwards]

the getLogString implementation is less broad than ours in register, because there are a lot of internal types that we don't want to lift here that we handle.

My idea was that we could keep using our Loggable as SquareLoggable for those, if desired.

[rjrjr]

That's a good idea. If it feels good once you've tried it out could suggest it in the kdoc here.

[rjrjr]

I wonder how many of our internal types we could get out of the big if / else if they implemented Unwrappable?

[rjrjr]

"log lines"

[steve-the-edwards]

Done.

[rjrjr]

• The kdoc on RuntimeUpdateLogLine is more informative, should probably say a lot of that here instead / too.
• We can suggest a useful spot to call this from (see below).
• Be good to update the kdoc of reportNavigation to draw attention to this, or if dependencies allow it add this to the default onNavigate param value.

* Consider calling this from the `onNavigate` function you provide to
* `reportNavigation`, e.g.
*
*         val renderings: Flow<Screen> by lazy {
*           renderWorkflowIn(
*             workflow = RootNavigationWorkflow,
*             scope = viewModelScope,
*             savedStateHandle = savedState,
*             runtimeConfig = RuntimeConfigOptions.ALL
*           ).reportNavigation {
*             runtimeMonitor.addRuntimeUpdate(
*               UiUpdateLogLine(getWfLogString(it))
*             )
*           }

[steve-the-edwards]

btw, TIL about NavigationMonitor!

[rjrjr]

Yeah, I snuck that out when updating the tutorials. I'm proud to report that we use it in prod, it's solid.

[rjrjr]

It can't b/c this is final, so "should not" is an odd thing to say.

[steve-the-edwards]

I guess i was just diary-ing my thoughts rather than making clear kdoc. Will change.

[rjrjr]

Is this direct tie to papa going to cause dependency hell down the road? Can we introduce a separate module that ties to papa, similar to what we did for radiography?

[steve-the-edwards]

sure, we could

[steve-the-edwards]

Done. separate out into workflow-tracing-papa