Preliminary support for tracing workflow activity

[mxgrey]

This PR introduces basic support for tracing the activity of operations in a workflow. I call it preliminary because there are a few details that could be improved upon:

• OperationRef is being used to uniquely identify the operations mentioned in the trace, but this might not be the most ergonomic representation for external consumers such as UIs. This is worth discussing further.
• This does not introduce tracing for the Listen operation, although any operation downstream of Listen will be traced.
• This does not provide an ergonomic API for tracing workflows built with the native API (traces are only added automatically via the diagram builder), although there is nothing to stop a user from adding Trace components manually.
• This does not provide any indication of how long an operation is active after it has started; it only triggers an event each time the operation starts.

In the interest of unblocking progress on the tracing capabilities of the workflow editor, I think the above issues can be left for follow-up work. The capabilities introduced in this PR should be enough to move forward with a proof of concept in the workflow editor.

Note that, related to this comment, I've changed the name: fields for node and section builders to default_display_text:, and I've added display text override options for all diagram operations.

To get a sense of how to read the traces, I recommend looking at the record_traces system in the trace.rs test module.

[koonpeng]

If an app does not have the trace feature, then it will cause the schema to diverge since true is no longer a valid value. I think it would be better if either

• Always have tracing available.
• If trace is an optional feature, do not throw an error, instead just print a warning.

---

I think we should also add a field to DiagramElementRegistry to indicate if tracing is supported.

[mxgrey]

I don't want to make tracing always required since it does add slight overhead to some potentially very hot code paths.

I wasn't sure whether to go with an error or a warning. I decided to go with error in case the user's workflow strongly depends on tracing being available, but you make a good point that the schema becomes invalid if we do it that way.

I like the idea of specifying the support in the DiagramElementRegistry. I think at that point we can just silently ignore if the user asks for tracing when we don't support it, because we've already communicated that it won't be available. At that point it's up to the front-end to make sure the user knows this.

I've made the relevant changes in this commit: 725be7c

[koonpeng]

Not super aware of the best practices when it comes to Arc, but I think generally for a constructor function, it is more preferred to take a value and do the clone on the caller side.

[mxgrey]

It's highly situational as there are advantages and disadvantages both ways. For example, if you always take by Arc<T> but then don't actually capture the value due to conditional logic, then you've forced the user to do a clone when it wasn't needed.

In this case, these methods are being called in many places, so if we took by Arc<T> then we're just needlessly forcing ourselves to put .clone() or Arc::clone(_) at all of those call sites. We also know in all of those cases that a clone will be necessary because none of those call sites can hand over ownership of the Arc<T> to this function. The only advantage to making it an Arc<T> argument is that if the call site can hand over ownership, then you prevent some unnecessary reference counting, but we know this is never the case for these methods, so we get no benefit from it.

Given the lack of any benefits for using Arc<T> arguments, I'd like to stick with &Arc<T>.

[koonpeng]

I think of it in another way. There is no branch in this constructor function that we do not clone. Taking &Arc has no advantage, even if the callee can give ownership, it will still be cloned (though the compiler can probably do some optimization). I think we also cannot make the assumption that all callee of this function will never be able to give ownership. This is a pub function, so that extends to all potential downstream crates. The only disadvantage I see is that it requires current callee to do an explicit clone, which imo is the rustic pattern anyway.

[koonpeng]

I think of it in another way. There is no branch in this constructor function that we do not clone. Taking &Arc has no advantage, even if the callee can give ownership, it will still be cloned (though the compiler can probably do some optimization). I think we also cannot make the assumption that all callee of this function will never be able to give ownership. This is a pub function, so that extends to all potential downstream crates. The only disadvantage I see is that it requires current callee to do an explicit clone, which imo is the rustic pattern anyway.

[mxgrey]

Taking &Arc has no advantage

If we go this route then we'll end up passing in Arc<JsonMessage> so then this concern would be gone.