Add duration output option to HarnessWithOutput

[dullbananas]

No description provided.

[weiznich]

Thanks for opening this PR. Overall I believe something like this would be a valuable addition. Mind explaining a bit the motivation for this change? I would be particular interested in clarifications to the following topics:

• Why did you choose to add everything to the existing harness instead of having a separate one for the new functionality?
• Why did you choose to tightly couple the implementation to tracing as opposed to something more flexible (like Instrumentation in diesel?)
• How would you like to test the new features? I don't see any tests for them as part of the PR, but I would like to have someone to ensure everything continues to work as expected.

[dullbananas]

The purpose of this change is to reduce the amount of custom code needed for making the harness show durations and output to tracing. Those are currently part of Lemmy's custom migration runner. Showing durations is useful for knowing which migrations are slow. Sending output to tracing is more normal than stdout in some projects, and Diesel currently makes it hard to do that because there's no Write implementation that would do that.

• Why did you choose to add everything to the existing harness instead of having a separate one for the new functionality?

The way I implemented it results in less duplication. A separate harness for tracing output would duplicate the timed option. A separate harness for the timed mode would duplicate the tracing constructors.

• Why did you choose to tightly couple the implementation to tracing as opposed to something more flexible (like Instrumentation in diesel?)

Outputting to tracing should require as little custom code as possible. A more flexible thing might be good to add later if the tracing constructors are kept as wrappers.

• How would you like to test the new features? I don't see any tests for them as part of the PR, but I would like to have someone to ensure everything continues to work as expected.

I now realize that HarnessWithOutput is indirectly tested by the diesel_cli tests, so I'm going to add a diesel_cli option to enable the timed mode. Also, Lemmy can be used to test both the timed mode and tracing output.

[weiznich]

Thanks for providing these details. The main reason why I'm asking is that we really do not want to add any new feature flags to diesel as that makes testing all variants much harder. The only exception here is if there is no other feasible option. That's not the case here as you can in fact have the same implementation outside of diesel.

Now that doesn't mean that I'm completely opposed to all the changes here. The timing part is definitely helpful for example. For the tracing part on the other hand I'm sceptical. Tracing is one of several solutions there, not everyone uses it and therefore we shouldn't for that dependency on others. Adding it as optional dependency would require a new feature flag, which in turn I want to avoid. Therefore the question to have something more generic instead. On the other hand that generic thing already exists as you easily can implement the MigrationHarness trait as third party crate and customize it however you want.

Also, Lemmy can be used to test both the timed mode and tracing output.

That's not no helpful as it requires running a lot third party code. I would like to have something like a unit or integration test for new features.

[dullbananas]

I won't be adding tracing support in this PR anymore. Adding a generic thing in diesel to make tracing easier to use might be a good idea to do later, but for now I will implement a different solution. I'm now working on the CLI option for timed mode.