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

telemetry: introduce orb-telemetry crate and use it in orb-ui #258

Merged
merged 1 commit into from
Nov 12, 2024

Conversation

TheButlah
Copy link
Collaborator

@TheButlah TheButlah commented Oct 10, 2024

we should in general be using journald for our logs, not stdout. Also, we should use stderr instead of stdout.

Every one of our crates sets up logging in a different way, but they all do the same thing. I wanted to introduce a single spot that lets us configure our telemetry. If it goes well in orb-ui, we can use it in all the rest too

@TheButlah TheButlah changed the title ui: use journald telemetry: introduce orb-telemetry crate and use it in orb-ui Oct 10, 2024
@TheButlah TheButlah requested a review from fouge October 10, 2024 00:59
@TheButlah TheButlah added difficulty:nontrivial This PR is not trivial, and requires more detailed review tested:no This PR was not tested labels Oct 10, 2024
@TheButlah TheButlah force-pushed the ryanbutler-orbp-289-orb-ui-should-use-journald branch from 0c2fc42 to 6fceae9 Compare October 22, 2024 20:58
Copy link
Collaborator

@fouge fouge left a comment

Choose a reason for hiding this comment

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

I tested and the filtering doesn't seem to work as expected

I have RUST_LOG=debug in my environment, but the same applies if the default_directive uses LevelFilter::DEBUG:

  • when filtering out info and above levels with journalctl, i still have debug logs (ie tracing::debug!())
  • these disappear only when I filter out warning and above levels 🤔
(venv) (os-b5c3a10) worldcoin@id-faff46c4:/mnt/scratch$ jl -fu worldcoin-ui --priority=info
-- Logs begin at Tue 2023-11-21 21:10:22 UTC. --
Oct 23 08:43:17 localhost worldcoin-ui[2338]: UI event: {"BatteryIsCharging":{"is_charging":true}}
[...]

corresponding line

tracing::debug!("UI event: {}", serde_json::to_string(event)?.as_str());

Comment on lines +49 to +50
/// Calling this more than once or when another tracing subscriber is registered
/// will cause a panic.
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we prevent the panic? return an error instead?

Copy link
Collaborator Author

@TheButlah TheButlah Oct 23, 2024

Choose a reason for hiding this comment

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

I think the panic is preferred. This type of code is once-only initialization code. If we called it more than once we should abort.

Generally errors are more for things that actually are expected to happen and need to be robustly handled. But someone calling this twice is better immediately slapped on the wrist and told to change their code.

@TheButlah
Copy link
Collaborator Author

TheButlah commented Oct 23, 2024

when filtering out info and above levels with journalctl, i still have debug logs

This is expected, journald has a different naming scheme than the tracing crate. See here. Info level messages in journald are debug level in tracing.
Screenshot 2024-10-23 at 14 57 26

@fouge
Copy link
Collaborator

fouge commented Oct 25, 2024

should we document this somewhere? 🤔

@TheButlah
Copy link
Collaborator Author

Its already documented by tracing_journald.

@TheButlah TheButlah requested a review from fouge November 9, 2024 23:40
@TheButlah TheButlah force-pushed the ryanbutler-orbp-289-orb-ui-should-use-journald branch from 6fceae9 to d979d84 Compare November 9, 2024 23:40
@TheButlah TheButlah enabled auto-merge (squash) November 9, 2024 23:41
@TheButlah TheButlah merged commit 9a1341e into main Nov 12, 2024
11 checks passed
@TheButlah TheButlah deleted the ryanbutler-orbp-289-orb-ui-should-use-journald branch November 12, 2024 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty:nontrivial This PR is not trivial, and requires more detailed review tested:no This PR was not tested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants