-
Notifications
You must be signed in to change notification settings - Fork 112
Upstreaming Runtime Monitoring and Tracing #1406
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
Conversation
| * to customize their logging names for [WorkflowRuntimeMonitor] output. | ||
| */ | ||
| public interface Loggable { | ||
| public fun toLogString(): String = toString().wfStripSquarePackage() |
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.
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?
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.
we can remove the default.
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.
Wise, it's a terrible default anyway. Shouldn't be making it so easy to use toString() by accident, defeats the purpose.
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.
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.
| /** | ||
| * Used as a key to detect actions originating from the Workflow that implements a worker. | ||
| */ | ||
| // Keep this up-to-date with the action in com.squareup.workflow1.WorkerWorkflow. |
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 have a unit test that will break if we screw that up?
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.
No. but we should. I will add.
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.
Did one better and just made it a constant in Worker.kt shared in both places
| /** | ||
| * Alternative to [toString] used by Workflow logging. | ||
| */ | ||
| internal fun getWfLogString(log: Any?): 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.
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.
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 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.
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.
Better to err on the side of privacy probably. Easier to open up later.
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.
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.
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.
That's a good idea. If it feels good once you've tried it out could suggest it in the kdoc here.
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 wonder how many of our internal types we could get out of the big if / else if they implemented Unwrappable?
| import com.squareup.workflow1.tracing.ActionAppliedLogLine.WorkflowActionLogType.WORKER_OUTPUT | ||
|
|
||
| /** | ||
| * PLEASE NOTE: these logs lines are turned into strings in production, all the time, and there |
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.
"log lines"
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.
Done.
| public var currentRenderCause: RenderCause? | ||
|
|
||
| /** | ||
| * Add an update into the [RuntimeUpdates] tracked by [WorkflowRuntimeMonitor]. |
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.
- The kdoc on
RuntimeUpdateLogLineis 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
reportNavigationto draw attention to this, or if dependencies allow it add this to the defaultonNavigateparam 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))
* )
* }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.
btw, TIL about NavigationMonitor!
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 snuck that out when updating the tutorials. I'm proud to report that we use it in prod, it's solid.
| /** SECTION: [WorkflowInterceptor] overrides. */ | ||
|
|
||
| /** | ||
| * [WorkflowRuntimeTracer]s should not override this method, using [onWorkflowSessionStarted] |
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.
It can't b/c this is final, so "should not" is an odd thing to say.
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 guess i was just diary-ing my thoughts rather than making clear kdoc. Will change.
workflow-tracing/build.gradle.kts
Outdated
| api(libs.androidx.collection) | ||
| api(libs.kotlin.jdk8) | ||
| api(libs.kotlinx.coroutines.core) | ||
| api(libs.squareup.papa) |
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.
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?
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.
sure, we could
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.
Done. separate out into workflow-tracing-papa
7b71c90 to
40ad34f
Compare
Upstream from our internal project; including with papa tracing. Adds a bunch of tests for verification of behaviour.
40ad34f to
629bbe5
Compare
| * By default this uses [PapaSafeTrace] which will use [androidx.tracing.Trace] calls that | ||
| * will be received by the system and included in Perfetto traces. | ||
| */ | ||
| class WorkflowPapaTracer( |
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.
This has the potential to be more of a generalized Tracer, where anything that has extended SafeTraceInterface can work here.
SafeTraceInterface has the workings of what could be a generalized approach with its current functions.
Might need renaming but that could be done in the future.
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.
ya, we can iterate. not many users (only us) of this for now.
| * The runtime has finished its work and is stable again - either skipping rendering because | ||
| * of no change ([RenderPassSkipped]), or having passed a new rendering ([RenderingProduced]). | ||
| */ | ||
| public data object RuntimeLoopTick : RuntimeUpdate |
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.
Nit: The name could be more descriptive, maybe RuntimeSettled?
| val csrConfig = config.contains(CONFLATE_STALE_RENDERINGS) | ||
| val ptrConfig = config.contains(PARTIAL_TREE_RENDERING) | ||
| val deaConfig = config.contains(DRAIN_EXCLUSIVE_ACTIONS) | ||
| val sehConfig = config.contains(STABLE_EVENT_HANDLERS) | ||
| val wsdConfig = config.contains(WORK_STEALING_DISPATCHER) |
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.
These properties are public API, they should probably not use abbreviations.
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
RuntimeUpdatetype that is clearer about when the runtime loop is finished (regardless of if the rendering is skipped or not).First commit removes the old
TracingWorkflowInterceptorand the correspondingtrace-encodermodule.The second commit adds the
WorkflowRuntimeMonitoras well as a bunch of reflection-less logging utilities that we've used for some time.