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

add testing environment indicator to the logger #2042

Closed
wants to merge 2 commits into from

Conversation

dsweber2
Copy link
Contributor

@dsweber2 dsweber2 commented Aug 29, 2024

Description

Following a recommendation from the structlog docs on testing. Needed so that I can actually test the log output from various warnings and errors in developing the Wastewater endpoint cmu-delphi/delphi-epidata#1513. Marked as a draft because I am still testing that this is all that's needed to be able to test the logger.

Changelog

  • Adds another option to get_structured_logger(name, filename, log_exceptions, *is_real=True*) indicating whether to log as if in a testing environment
  • A bunch of formatting things that ruff decided were Correct. Can remove if they're a problem.

@aysim319
Copy link
Contributor

@dsweber2 I would suggest adding @minhkhul as a reviewer, I know that she's working with some elastic and logging stuff I feel like she would be able to give some insight on that part

@dsweber2
Copy link
Contributor Author

Mostly unrelated, but I would definitely prefer if we dealt with formatting stuff all in one PR.

@minhkhul
Copy link
Contributor

If in your case, test runs are 1. also run on prod, and 2. the logged info must be output to the same log file as real runs, then the is_real addition makes sense.
But elastic allows us to distinguish different kinds of run using server names and/or the dir where log are output to. So far, that's been enough to help distinguish things. I think it's a good idea to separate where we put real log files and test log files anyway.
So instead of adding a flag, I suggest doing those 2 things first if possible (run test on staging and/or output log of test to a separate location from real logging).

If you want to, you can set up an elastic ingest pipeline to take advantage of elastic's log parsing by doing these:

  1. Put your test log file into something like /var/log/test-wastewater/
  2. Set up an elastic ingest pipeline like this
  3. Add pickup configuration like this so your test log are automatically picked up and put into the ingestion pipeline you created in step 3.
  4. Then you can make dashboard like this

@melange396
Copy link
Contributor

@minhkhul the point of this is to make it easier to evaluate the application's logging functionality in a sandboxed test environment; Elastic isnt part of the scope.

I would lean towards removing the formatting changes from this PR, they distract from the core diffferences being applied and the rationale.

def get_structured_logger(name=__name__,
filename=None,
log_exceptions=True):
def get_structured_logger(name=__name__, filename=None, log_exceptions=True, is_real=True):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
def get_structured_logger(name=__name__, filename=None, log_exceptions=True, is_real=True):
def get_structured_logger(name=__name__, filename=None, log_exceptions=True, *, test_mode=False):

and then below:

cache_logger_on_first_use=not(test_mode),

This name (and inverted truth value) makes the purpose of the argument clearer, and the * in the arguments list ensures its only ever triggered explicitly.

@dsweber2
Copy link
Contributor Author

dsweber2 commented Sep 3, 2024

Thanks all for the comments, I eventually figured out a way around it without having to modify the functionality of get_structured_logger while using pytests.

@dsweber2 dsweber2 closed this Sep 3, 2024
@dshemetov dshemetov deleted the dsweber2/testingLogger branch September 4, 2024 00:19
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.

5 participants