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

Remove trace logging #6103

Merged
merged 1 commit into from
Jul 16, 2024
Merged

Remove trace logging #6103

merged 1 commit into from
Jul 16, 2024

Conversation

AgeManning
Copy link
Member

In practice I do not use trace logging and I don't know if anyone else does.

The way slog works is it passes these logs into its own task and then filters based on the log level. We are currently compiling with trace logging, which I would imagine has a non-negligible performance hit by producing all the trace logging then filtering them out.

Even when we are debugging we rarely use trace-level logging (to my knowledge). Even if we wanted to use them, we would have to restart the client as most default configurations favour debug-level.

For those really wanting trace-level logging, I propose they re-compile lighthouse to enable it, to spare everyone else not using tracing from generating and filtering the logs.

@AgeManning AgeManning added the ready-for-review The code is ready for review label Jul 16, 2024
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Yeah I support this. I would even be on board with deleting all uses of trace! eventually, due to how useless they are.

Usually if we want to debug something, we'll add a debug log.

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. backwards-incompat Backwards-incompatible API change and removed ready-for-review The code is ready for review labels Jul 16, 2024
@michaelsproul
Copy link
Member

I think this is a breaking change because users who are running with --debug-level trace will get an error.

@AgeManning
Copy link
Member Author

I think this is a breaking change because users who are running with --debug-level trace will get an error.

Will they get an error, or will it just not output anything?

@michaelsproul
Copy link
Member

I'll compile and test it

@michaelsproul
Copy link
Member

Ok yeah it's fine, it doesn't error, it just drops the trace logs.

@michaelsproul michaelsproul removed the backwards-incompat Backwards-incompatible API change label Jul 16, 2024
@michaelsproul
Copy link
Member

@mergify queue

Copy link

mergify bot commented Jul 16, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 77d491b

mergify bot added a commit that referenced this pull request Jul 16, 2024
@mergify mergify bot merged commit 77d491b into sigp:unstable Jul 16, 2024
28 checks passed
@dapplion
Copy link
Collaborator

The instance of useful trace logs I have seen is in debugging sync tests, where the log is valuable on a single run of sync 1 block but would be to noisy if enabled in prod. We could replace the trace log with

#[cfg(test)]
debug!()

ThreeHrSleep pushed a commit to ThreeHrSleep/lighthouse that referenced this pull request Aug 1, 2024
* Remove trace logging
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants