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

Tracing improvments #124

Merged
merged 7 commits into from
Jul 6, 2023
Merged

Tracing improvments #124

merged 7 commits into from
Jul 6, 2023

Conversation

PetrichorIT
Copy link
Contributor

Some improvements to tracing with turmoil:

  • All nodes now have a node-span, to add information about the currently active node to all tracing events
  • These spans are stored in Rt and Host to cover all codepaths that may emit events
  • All internal tracing events now share a common structure (src,dst, ..., msg)
  • src now always represents the source of the packet, the tracing record was about (src of SYN is client, src of SYN-ACK is server)

@davidbarsky
Copy link
Member

Generally speaking, you shouldn't need to pass a tracing::Span to usage sites: you can create a new one at the usage site and tracing will figure out the appropriate parentage/hierarchy automatically.

@mcches
Copy link
Contributor

mcches commented Jun 20, 2023

Tagging #49.

@PetrichorIT
Copy link
Contributor Author

@davidbarsky since spans are entered 3 times pre tick, it seems wasteful to me to create the span inline,
since they require custom fields generated from a reverse dns request each time. The core idea behind passing
the span to the appropriate usage site is to minimize dns queries and string allocation. Would you consider the performance impact of span creation significant enough to warrant this unusual structure or should we just switch to inline-creation of spans, say with a shared Arc<String> to minimize reverse dns queries?

@davidbarsky
Copy link
Member

Would you consider the performance impact of span creation significant enough to warrant this unusual structure or should we just switch to inline-creation of spans, say with a shared Arc<String> to minimize reverse dns queries?

Creating spans and events is generally pretty cheap, especially if they are disabled and filtered out. I think keeping an Arc<str> around, especially if it's expensive to fetch, is perfectly reasonable.

Copy link
Contributor

@mcches mcches left a comment

Choose a reason for hiding this comment

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

A couple minor comments. This will be really nice for debugging.

src/top.rs Outdated Show resolved Hide resolved
src/sim.rs Outdated Show resolved Hide resolved
@mcches mcches linked an issue Jul 6, 2023 that may be closed by this pull request
@mcches mcches merged commit 5954ef3 into tokio-rs:main Jul 6, 2023
3 checks passed
@PetrichorIT PetrichorIT deleted the tracing branch July 27, 2023 10:49
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.

Look into spans for network tracing
3 participants