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

Use tracing levels for different network events #53

Merged
merged 1 commit into from
Nov 9, 2022
Merged

Use tracing levels for different network events #53

merged 1 commit into from
Nov 9, 2022

Conversation

mcches
Copy link
Contributor

@mcches mcches commented Nov 9, 2022

Everything was at info, which got pretty noisy. We now use
tracing directly and leverage level to differentiate based on
event frequency.

Less frequent events (like new host or bind) use info and normal
network traffic (like send/hold/recv etc.) use trace.

Work still needs to be done here to integrate spans per #49.

Everything was at `info`, which got pretty noisy. We now use
`tracing` directly and leverage level to differentiate based on
event frequency.

Less frequent events (like new host or bind) use `info` and normal
network traffic (like send/hold/recv etc.) use `trace`.

Work still needs to be done here to integrate spans per #49.
@@ -85,7 +85,7 @@ impl Host {
pub(crate) fn receive_from_network(&mut self, envelope: Envelope) -> Result<(), Protocol> {
let Envelope { src, dst, message } = envelope;

trace!(?dst, ?src, protocol = %message, "Delivered");
tracing::trace!(target: TRACING_TARGET, ?dst, ?src, protocol = %message, "Delivered");
Copy link
Member

Choose a reason for hiding this comment

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

Isnt the target already set since the crate name is turmoil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is the full mod path, which isn't major but it's pretty nice to just see turmoil on all the events.

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, @davidbarsky I wonder what you think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per #49 I assume everything that went to trace in this change would be a span. I don't like seeing the internal mod path in the traces, which is why I use a constant. It's not really a trace for the crate, rather it's a network trace that we happen to use tracing for. I don't feel too strongly though, just personal preference. Long term, I think we want a subscriber included that does nice things like write in different formats, and to different sinks like a file.

Copy link
Member

Choose a reason for hiding this comment

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

sorry, i was offline yesterday for reasons (i still have a job thankfully, but I took a personal day).

Interesting, @davidbarsky I wonder what you think

@LucioFranco when i was talking to @mcches over discord, my feeling is that i'm eh about changing the target (overriding targets is a feature that is seldom used, and I expect that to remain the case until const evaluation develops some very advanced features, but that's out of scope for this discussion and it's better that turmoil expose/ship with a custom formatter or layer that does the things/formatting that Brett wants to do, but given the deadlines he mentioned, i feel that it's better to not let perfect become the enemy of good enough.

@mcches mcches merged commit 5b520a3 into main Nov 9, 2022
@mcches mcches deleted the trace branch November 9, 2022 19:09
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.

None yet

3 participants