cmu-delphi / covidcast-indicators

Back end for producing indicators and loading them into the COVIDcast API.
https://cmu-delphi.github.io/delphi-epidata/api/covidcast.html
MIT License
12 stars 17 forks source link

add testing environment indicator to the logger #2042

Closed dsweber2 closed 2 months ago

dsweber2 commented 2 months ago

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 https://github.com/cmu-delphi/delphi-epidata/pull/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

aysim319 commented 2 months ago

@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 commented 2 months ago

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

minhkhul commented 2 months ago

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 commented 2 months ago

@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.

dsweber2 commented 2 months ago

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.