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
Open

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

Show file tree
Hide file tree
Changes from all commits
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
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
5 changes: 3 additions & 2 deletions src/events/chatlist_events.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,10 @@ pub(crate) async fn emit_chatlist_item_changed_for_contact_chat(
match ChatId::lookup_by_contact(context, contact_id).await {
Ok(Some(chat_id)) => self::emit_chatlist_item_changed(context, chat_id),
Ok(None) => {}
Err(error) => context.emit_event(EventType::Error(format!(
Err(error) => error!(
context,
"failed to find chat id for contact for chatlist event: {error:?}"
))),
),
}
}

Expand Down
5 changes: 5 additions & 0 deletions src/log/stream.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,11 @@ impl<S: SessionStream> AsyncRead for LoggingStream<S> {
"Read error on stream {peer_addr:?} after reading {} and writing {} bytes: {err}.",
this.metrics.total_read, this.metrics.total_written
);
tracing::event!(
::tracing::Level::WARN,
account_id = *this.account_id,
log_message
);
this.events.emit(Event {
id: *this.account_id,
typ: EventType::Warning(log_message),
Expand Down
6 changes: 3 additions & 3 deletions src/qr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ use serde::Deserialize;
use crate::config::Config;
use crate::contact::{Contact, ContactId, Origin};
use crate::context::Context;
use crate::events::EventType;
use crate::key::Fingerprint;
use crate::login_param::{EnteredCertificateChecks, EnteredLoginParam, EnteredServerLoginParam};
use crate::net::http::post_empty;
Expand Down Expand Up @@ -748,9 +747,10 @@ pub(crate) async fn login_param_from_account_qr(
match serde_json::from_str::<CreateAccountErrorResponse>(&response_text) {
Ok(error) => Err(anyhow!(error.reason)),
Err(parse_error) => {
context.emit_event(EventType::Error(format!(
error!(
context,
"Cannot create account, server response could not be parsed:\n{parse_error:#}\nraw response:\n{response_text}"
)));
);
bail!("Cannot create account, unexpected server response:\n{response_text:?}")
}
}
Expand Down