application-services icon indicating copy to clipboard operation
application-services copied to clipboard

Add adr-9: Replace logging and error reporting infrastructure with tracing

Open mhammond opened this issue 1 year ago • 6 comments

Rendered

Putting this up as a draft for internal feedback, after which I'll share it more widely (but feel free to share it as you see fit and/or add additional reviewers)

mhammond avatar Mar 06 '24 22:03 mhammond

Thanks for the comment so far - I pushed a new commit with the suggestions here.

mhammond avatar Mar 08 '24 21:03 mhammond

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 84.08%. Comparing base (ec40bd6) to head (8f16a78).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6157   +/-   ##
=======================================
  Coverage   84.08%   84.08%           
=======================================
  Files         117      117           
  Lines       15629    15629           
=======================================
  Hits        13141    13141           
  Misses       2488     2488           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Mar 08 '24 21:03 codecov-commenter

One key different in this model which I need to call out in the doc is that something needs to opt-in to each target. eg, in the logging world, you can arrange to see logs from, say, dogear, without taking any specific action regarding that crate. However, in this model, something is going to need to explicitly opt-in to the dogear target by creating an event sink for it.

On one hand that's quite a PITA, but OTOH, this is really what makes this viable - there's really no concept of a "global listener for all events".

Does that change the balance for anyone?

mhammond avatar Mar 11 '24 18:03 mhammond

However, in this model, something is going to need to explicitly opt-in to the dogear target by creating an event sink for it.

That all makes sense to me, and I like the new behavior change!

linabutler avatar Mar 11 '24 19:03 linabutler

However, in this model, something is going to need to explicitly opt-in to the dogear target by creating an event sink for it.

That all makes sense to me, and I like the new behavior change!

Me as well. The text changes are also looking great. Should we send this out to a larger audience?

bendk avatar Mar 12 '24 13:03 bendk

This looks great! Just wanted to add some extra context here :)

We've been using Tracing extensively in various Rust services, and here is a similar ADR we did for Merino (Rust) a while back. I think the main selling points to service engineers are its better logging support (spans, events, and instrument) in the asynchronous environments and the widespread adoption in the Tokio ecosystem. Of course, it should work equally well for non-asynchronous use cases as well.

So, as another minor pro, the adoption of Tracing will make folks with past experience on Tracing feel right at home when they work on app-services.

ncloudioj avatar Mar 12 '24 14:03 ncloudioj