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

ref(iroh): Rename target for events #2977

Merged
merged 2 commits into from
Nov 29, 2024
Merged

ref(iroh): Rename target for events #2977

merged 2 commits into from
Nov 29, 2024

Conversation

flub
Copy link
Contributor

@flub flub commented Nov 28, 2024

Description

Turns out people will configure logs with RUST_LOG=iroh=debug and then the events target is completely lost. That's bad!

This changes the events to be in the main crate target, so users don't have to separately enable them.

It also changes the separators to use the ::. The driver for this is compatible with the EnvFliter syntax, allowing you do do e.g. RUST_LOG=iroh::events=debug to enable just events.

The main downside is that it is harder to enable events across different crates. But then we only use this in one crate so far, so perhaps that's not an issue yet.

Another downside is that iroh::events could quite easily become a module name, in which case the targets would conflict.

Breaking Changes

  • Events are emitted on different tracing targets: iroh::events instead of events.net.
  • Event names have changed to use :: as separator, previously this was ..

Notes & open questions

Perhaps there is a better name than events that is less likely to conflict with a real module? Maybe we could use an illegal character? Experimenting with iroh::-events:: as prefix does seem to work: it is accepted by the tracing and the EnvFilter is fine with it.

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.

Turns out people will configure logs with `RUST_LOG=iroh=debug` and
then the `events` target is completely lost.  That's bad!
Copy link

github-actions bot commented Nov 28, 2024

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2977/docs/iroh/

Last updated: 2024-11-29T09:43:57Z

Copy link

github-actions bot commented Nov 28, 2024

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: 2e7b44c

@flub flub marked this pull request as ready for review November 29, 2024 08:54
@rklaehn rklaehn requested review from rklaehn and matheus23 November 29, 2024 09:07
Copy link
Contributor

@rklaehn rklaehn left a comment

Choose a reason for hiding this comment

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

If this works for you, go for it. However, I find it a bit weird that these things look like module paths but don't actually match the modules the events are emitted in.

Given the latter, maybe having a special "module name" for the events pseudomodule would be nice to distinguish these from normal module paths.

@flub
Copy link
Contributor Author

flub commented Nov 29, 2024

If this works for you, go for it. However, I find it a bit weird that these things look like module paths but don't actually match the modules the events are emitted in.

The tracing metadata also has a file field which records the file. This is however not shown by the default fmt subscriber. Also, a file name is not 1:1 mapped to a module name, so yeah. So far I have been trying to compromise between practicality and enough distinguishing semantics that you can process them automatically.

Given the latter, maybe having a special "module name" for the events pseudomodule would be nice to distinguish these from normal module paths.

Copy link
Contributor

@matheus23 matheus23 left a comment

Choose a reason for hiding this comment

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

This makes sense to me.

Perhaps there is a better name than events that is less likely to conflict with a real module?

I suggested _events in the discord. This both avoids a conflict with a potential module (we probably will never add an _events module), and also doesn't use a special character, so wouldn't confuse some tool that only expects valid module names.

@flub flub enabled auto-merge November 29, 2024 09:42
@flub flub added this pull request to the merge queue Nov 29, 2024
Merged via the queue into main with commit 43d0ea4 Nov 29, 2024
26 checks passed
@flub flub deleted the flub/events-in-crate branch November 29, 2024 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants