Skip to content
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

Add adr-8: Replace logging and error reporting infrastructure with tracing #6157

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mhammond
Copy link
Member

@mhammond mhammond commented Mar 6, 2024

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)

docs/adr/0008-use-tracing.md Outdated Show resolved Hide resolved
docs/adr/0008-use-tracing.md Outdated Show resolved Hide resolved
docs/adr/0008-use-tracing.md Show resolved Hide resolved
docs/adr/0008-use-tracing.md Outdated Show resolved Hide resolved
docs/adr/0008-use-tracing.md Outdated Show resolved Hide resolved

Desktop has a [hand-written xpcom-based log adaptor](https://searchfox.org/mozilla-central/source/services/sync/golden_gate/src/log.rs#119-120). This would be removed entirely and a uniffi-based callback mechanism is used. Rust code calling back into Javascript has the same semantics as `golden_gate` - the log calls are "fire and forget", ending up in the main thread automatically.

The [`gecko-logger`](https://searchfox.org/mozilla-central/source/xpcom/rust/gecko_logger/src/lib.rs) crate would change:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me so happy! 🙌🏼 I felt like the "log sink" pattern started out as a stopgap to connect Rust bookmark sync to Sync's logging machinery, and it was easy for us to generalize once we started adding more Rust components. It's wonderful to revisit it from the ground up 😁

docs/adr/0008-use-tracing.md Outdated Show resolved Hide resolved
@mhammond
Copy link
Member Author

mhammond commented Mar 8, 2024

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

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

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           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mhammond
Copy link
Member Author

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?

@linabutler
Copy link
Member

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!

@bendk
Copy link
Contributor

bendk commented Mar 12, 2024

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?

@ncloudioj
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants