Skip to content

fix: use logging macros instead of emitting event directly, so that it is also logged by tracing #7459

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+26 −5
Open

fix: use logging macros instead of emitting event directly, so that it is also logged by tracing #7459

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
76eb2e4
fix: use logging macros instead of emitting event directly, so that it
Simon-Laux Nov 16, 2025
06e6dc8
also emit tracing events on account events in methods that doen't use
Simon-Laux Nov 16, 2025
253b2b1
rm unused import
Simon-Laux Nov 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
also emit tracing events on account events in methods that doen't use 
the macro, because they can't capture self.
  • Loading branch information
@Simon-Laux
Simon-Laux committed Nov 16, 2025
commit 06e6dc8eb9f18dac54a498ace58b858f397c80b4
15 changes: 15 additions & 0 deletions src/accounts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,11 @@ impl Accounts {
"Starting background fetch for {n_accounts} accounts."
)),
});
::tracing::event!(
Copy link
Collaborator

@iequidoo iequidoo Nov 16, 2025

Choose a reason for hiding this comment

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

Can we add some function to Events that also does this, to avoid code duplication?

Copy link
Contributor Author

@Simon-Laux Simon-Laux Nov 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

normally it is inside of the logging macros, but the problem here is that we would need to either adapt the macro or duplicate and modify it to allow calling it without a structure/type with a .get_id(&self) method.
I don't felt confident doing either of those bigger changes/decisions.
The methods here are special, because they don't want to capture self.

Another idea could be a dummy struct that would provide a .get_id(&self) which just returns 0, but that feels like a hack to me.

Copy link
Collaborator

@iequidoo iequidoo Nov 16, 2025

Choose a reason for hiding this comment

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

I think we need a trait for ctx passed into logging macros. Currently Context is actually an account and also acts as a logging context, we should split these concepts. This is not a subject for this PR of course. Another benefit of such a logging context may be storing some logging prefix or activity id which is logged as a log line prefix. This way we can make logs structured which may be useful for multi-transport, otherwise it may not be clear in the context of which transport an event occurs.

::tracing::Level::INFO,
account_id = 0,
"Starting background fetch for {n_accounts} accounts."
);
let mut set = JoinSet::new();
for account in accounts {
set.spawn(async move {
Expand All @@ -375,6 +380,11 @@ impl Accounts {
"Finished background fetch for {n_accounts} accounts."
)),
});
::tracing::event!(
::tracing::Level::INFO,
account_id = 0,
"Finished background fetch for {n_accounts} accounts."
);
}

/// Auxiliary function for [Accounts::background_fetch].
Expand All @@ -393,6 +403,11 @@ impl Accounts {
id: 0,
typ: EventType::Warning("Background fetch timed out.".to_string()),
});
::tracing::event!(
::tracing::Level::WARN,
account_id = 0,
"Background fetch timed out."
);
}
events.emit(Event {
id: 0,
Expand Down
Loading