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

Allow disabling logging by setting AUSTIN_NO_LOGGING in the environment #121

Merged
merged 1 commit into from
Jul 5, 2022

Conversation

Mortal
Copy link
Contributor

@Mortal Mortal commented Jun 29, 2022

Add a check in logging.c for the environment variable AUSTIN_NO_LOGGING. If it is nonempty, logging is disabled on all platforms.

Alternate Designs

None considered.

Regressions

No possible side-effects, as the odds of existing users inadvertently setting AUSTIN_NO_LOGGING are minimal.

Verification Process

To test (on Linux):

  1. Compile with gcc -O3 -Os -Wall -pthread src/*.c -o src/austin
  2. Open a separate terminal to watch the syslog output with journalctl -f -t austin -n0
  3. Run AUSTIN_NO_LOGGING=1 ./src/austin python3 -c 'import time; time.sleep(1)' and observe that nothing is output to syslog
  4. Run AUSTIN_NO_LOGGING= ./src/austin python3 -c 'import time; time.sleep(1)' and observe the usual output to syslog
  5. Run ./src/austin python3 -c 'import time; time.sleep(1)' and observe the usual output to syslog

@P403n1x87
Copy link
Owner

👋 thanks for your contribution. This feature sounds interesting. I'm just curious, could you share with me your specific use case (while I figure out why the workflows are not running)? Also, it would be great if you could target the devel branch instead. Cheers!

@Mortal Mortal changed the base branch from master to devel June 30, 2022 10:21
@Mortal
Copy link
Contributor Author

Mortal commented Jun 30, 2022

As for my use case, we're running several networked daemons written in Python that do a lot of marshaling/unmarshaling and database queries, and during certain events we're getting really unsatisfactory performance. It would be nice if I could attach austin in our production environment and obtain some samples to indicate where to optimize. However I don't want to spew syslog in our production environments with austin's logging. If austin could log to stderr and output samples to stdout then maybe that could work for me as well, but I thought it would be easiest to just disable logging altogether.

@P403n1x87
Copy link
Owner

@Mortal thanks for sharing your use case. Since this is a new feature, and in fact the first supported environment variable, we would need to:

  • add an entry in the change-log
  • document the environment variable interface in the README
  • reference the new variable in the Logging section of the README
  • add simple fork and attach test cases with the variable set to ensure that we don't crash

As for the second entry in the list above, I would add the Environment Interface subsection under Usage in which we briefly say that some Austin behaviour can be configured via environment variables, and then add a table with the supported variables (just AUSTIN_NO_LOGGING in this case). Are you happy to also contribute these changes?

Also, just a heads-up that I might also ask you to rebase your branch on devel, once I push the GitHub Actions changes required to run on PRs.

@P403n1x87
Copy link
Owner

@Mortal PR #122 is now merged. Please rebase on devel when you can 🙏

@Mortal Mortal force-pushed the no_log branch 2 times, most recently from a62e0c3 to 49aa73f Compare July 4, 2022 08:35
@Mortal
Copy link
Contributor Author

Mortal commented Jul 4, 2022

@P403n1x87 I've rebased, updated the change-log and README, and added a test per your feedback. Please take another look. Also, it seems you need to give approval to run the workflows: First-time contributors need a maintainer to approve running workflows.

@codecov
Copy link

codecov bot commented Jul 4, 2022

Codecov Report

Merging #121 (3816045) into devel (d1e56c1) will decrease coverage by 0.11%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##            devel     #121      +/-   ##
==========================================
- Coverage   69.83%   69.71%   -0.12%     
==========================================
  Files          22       22              
  Lines        2347     2351       +4     
  Branches      655      660       +5     
==========================================
  Hits         1639     1639              
- Misses        411      414       +3     
- Partials      297      298       +1     
Impacted Files Coverage Δ
src/logging.c 87.75% <85.71%> (-1.14%) ⬇️
src/py_string.h 61.53% <0.00%> (-12.83%) ⬇️
src/py_proc_list.c 74.80% <0.00%> (-0.77%) ⬇️
src/py_proc.c 61.89% <0.00%> (+0.72%) ⬆️
src/py_thread.c 74.45% <0.00%> (+1.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0c8308...3816045. Read the comment docs.

Copy link
Owner

@P403n1x87 P403n1x87 left a comment

Choose a reason for hiding this comment

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

Some small comments and suggestions, otherwise LGTM!

README.md Outdated Show resolved Hide resolved
src/logging.c Outdated Show resolved Hide resolved
src/logging.c Outdated Show resolved Hide resolved
test/test_fork.py Outdated Show resolved Hide resolved
test/utils.py Outdated Show resolved Hide resolved
Copy link
Owner

@P403n1x87 P403n1x87 left a comment

Choose a reason for hiding this comment

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

🙌 thanks for your contribution!

@P403n1x87 P403n1x87 merged commit 3afc002 into P403n1x87:devel Jul 5, 2022
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