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

trace: Simplify dynamic tracing level application #380

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

jplatte
Copy link
Collaborator

@jplatte jplatte commented Jun 24, 2023

Motivation

I've been looking to refactor the trace layer (really kind of rewrite it, but I wanted to read most of the existing code and ideally transform it piece by piece instead of starting from scratch, which would likely lead to lots of going back and forth as I discover new things that the old code supported that aren't that easy).

As part of that, I found the log_pattern_match macros and thought that there had to be a better way. And I think I found a better way :)

Solution

  • Don't duplicate event! invocations for different format_args! strings, instead create a custom type that implements Display and create it on-the-fly to then log it
  • Don't duplicate event! invocations for when a field is supposed to be captured vs. not, instead make the field value an Option which has the same effect (not 100% the same I think, the FieldSet for the event will always contain the field now, but the ValueSet will still behave the same which should be the only practically relevant thing)
  • With all other differences gotten rid of, create a single macro for logging event!s with a non-const level

Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

awesome!

@davidpdrsn davidpdrsn merged commit 1cd1505 into master Jul 19, 2023
@davidpdrsn davidpdrsn deleted the jplatte/trace-simplify branch July 19, 2023 08:54
gezihuzi pushed a commit to gezihuzi/tower-http that referenced this pull request Jul 19, 2023
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.

2 participants