mirrord icon indicating copy to clipboard operation
mirrord copied to clipboard

feat: initial progress reporting

Open tamasfe opened this issue 2 years ago • 1 comments

Initial version of progress reporting via tracing spans, closes #389.

Reporting is done in 4 flavors:

  • standard mode: fancy reporting with spinners
  • simple mode: simple messages printed
  • json: not implmented
  • off: the layer is completely disabled

The tasks start when a span is first entered, and end when the span is closed (dropped), the reason for this is async code where a span is repeatedly entered whenever the futures are polled.

In standard mode the same spinner is reused for all nested spans, and the ongoing task is only done when the outermost span is closed.

standard mode example

https://user-images.githubusercontent.com/25967296/190286758-cbd82564-7f60-4fe5-8bc9-4b0d8b9cabd7.mp4

The agent setup ended up being too quick when I actually got to try it out so I had to add artificial network delays for the vid above...

I also introduced a mirrord-common library for stuff like this that are used from both the cli and the layer.

Todo

  • I cannot seem to pass env vars to the layer from the cli, so by default the layer has standard reporting enabled as well.
  • We only check if a span is closed, if it's closed, success is printed, even if the task at hand failed. I'm not sure how to handle this, I'd say it's probably not a huge issue as the error is immediately printed, but a false success message is still ugly or probably even confusing.
  • Each time I write a tracing layer I need to rediscover how tracing works, so the code is a bit messy and requires a cleanup.

tamasfe avatar Sep 15 '22 00:09 tamasfe

The example looks amazing! We'll try to review it ASAP!

aviramha avatar Sep 15 '22 16:09 aviramha

@tamasfe Hey, wanted to check if you were planning to get back to this one anytime soon? Feel free if you want us to pick it up where you left off.

eyalb181 avatar Sep 28 '22 15:09 eyalb181

@tamasfe Hey, wanted to check if you were planning to get back to this one anytime soon? Feel free if you want us to pick it up where you left off.

Sorry, I didn't mean to vanish. I felt burnt-out lately and avoided Github altogether.

I realized that this feature isn't really mirrord-specific, so I decided to overengineer it as a separate library for the following reasons:

  • No control over the thread that prints the spinner line
  • Only one line can be shown at a time
  • It's not possible to lock stdout, so the spinner might overwrite messages

My library allows for multiple lines at the same time while retains all their state and allows for finer control so that a thread is not always running for no reason. I also wanted indentation of subtasks.

Also I'm not sure if tracing is the right approach anymore. Yes, it's somewhat convenient, but at the same time it was not designed for this use-case. Once a subscriber is registered, it cannot be removed. I think progress reporting like this is niche enough not to be part of the main logging library.

I don't think I'll get back to this before next week. If you like this approach I can continue then or polish the mentioned library and open a separate PR with that. Either way, feel free to take it over if you have the capacity, I don't want to hold anyone up.

tamasfe avatar Sep 28 '22 23:09 tamasfe

We're in no rush to get this merged, so if you're planning to get back to it sometimes in the near future we'll leave it to you. I think we do like the current approach.

eyalb181 avatar Oct 03 '22 15:10 eyalb181

Closing in favor of #537.

tamasfe avatar Oct 10 '22 21:10 tamasfe