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

added progress chunk to limit logging #2024

Merged
merged 3 commits into from
Aug 15, 2024
Merged

Conversation

aysim319
Copy link
Contributor

@aysim319 aysim319 commented Aug 14, 2024

Description

too much logging for the progress

hopsital-admission

{"time": "datetime.datetime(2024, 8, 14, 14, 37, 55, 187263)", "event": "starting download", "logger": "delphi_claims_hosp.run", "level": "info", "pid": 68373, "timestamp": "2024-08-14T18:37:55.187298Z"}
{"filename": "EDI_AGG_INPATIENT_20240813_1452CDT.csv.gz", "event": "File to download", "logger": "delphi_claims_hosp.run", "level": "info", "pid": 68373, "timestamp": "2024-08-14T18:37:58.514091Z"}
{"filename": "EDI_AGG_INPATIENT_20240814_0252CDT.csv.gz", "event": "File to download", "logger": "delphi_claims_hosp.run", "level": "info", "pid": 68373, "timestamp": "2024-08-14T18:37:58.514295Z"}
{"filename": "EDI_AGG_INPATIENT_20240813_1452CDT.csv.gz", "percent": 0, "event": "Transfer in progress", "logger": "delphi_claims_hosp.run", "level": "info", "pid": 68373, "timestamp": "2024-08-14T18:37:58.587617Z"}
{"filename": "EDI_AGG_INPATIENT_20240813_1452CDT.csv.gz", "percent": 25, "event": "Transfer in progress", "logger": "delphi_claims_hosp.run", "level": "info", "pid": 68373, "timestamp": "2024-08-14T18:38:02.373432Z"}
{"filename": "EDI_AGG_INPATIENT_20240813_1452CDT.csv.gz", "percent": 50, "event": "Transfer in progress", "logger": "delphi_claims_hosp.run", "level": "info", "pid": 68373, "timestamp": "2024-08-14T18:38:05.321412Z"}
{"filename": "EDI_AGG_INPATIENT_20240813_1452CDT.csv.gz", "percent": 75, "event": "Transfer in progress", "logger": "delphi_claims_hosp.run", "level": "info", "pid": 68373, "timestamp": "2024-08-14T18:38:07.928315Z"}
{"filename": "EDI_AGG_INPATIENT_20240813_1452CDT.csv.gz", "percent": 100, "event": "Transfer in progress", "logger": "delphi_claims_hosp.run", "level": "info", "pid": 68373, "timestamp": "2024-08-14T18:38:10.895372Z"}
{"filename": "EDI_AGG_INPATIENT_20240814_0252CDT.csv.gz", "percent": 0, "event": "Transfer in progress", "logger": "delphi_claims_hosp.run", "level": "info", "pid": 68373, "timestamp": "2024-08-14T18:38:10.985207Z"}
{"filename": "EDI_AGG_INPATIENT_20240814_0252CDT.csv.gz", "percent": 25, "event": "Transfer in progress", "logger": "delphi_claims_hosp.run", "level": "info", "pid": 68373, "timestamp": "2024-08-14T18:38:14.024649Z"}
{"filename": "EDI_AGG_INPATIENT_20240814_0252CDT.csv.gz", "percent": 50, "event": "Transfer in progress", "logger": "delphi_claims_hosp.run", "level": "info", "pid": 68373, "timestamp": "2024-08-14T18:38:17.388245Z"}
{"filename": "EDI_AGG_INPATIENT_20240814_0252CDT.csv.gz", "percent": 75, "event": "Transfer in progress", "logger": "delphi_claims_hosp.run", "level": "info", "pid": 68373, "timestamp": "2024-08-14T18:38:20.633262Z"}
{"filename": "EDI_AGG_INPATIENT_20240814_0252CDT.csv.gz", "percent": 100, "event": "Transfer in progress", "logger": "delphi_claims_hosp.run", "level": "info", "pid": 68373, "timestamp": "2024-08-14T18:38:23.601564Z"}

doctor-visits

{"time": "datetime.datetime(2024, 8, 14, 14, 39, 49, 406542)", "event": "starting download", "logger": "delphi_doctor_visits.run", "level": "info", "pid": 68402, "timestamp": "2024-08-14T18:39:49.406580Z"}
{"filename": "EDI_AGG_OUTPATIENT_20240813_1452CDT.csv.gz", "event": "File to download", "logger": "delphi_doctor_visits.run", "level": "info", "pid": 68402, "timestamp": "2024-08-14T18:39:53.062629Z"}
{"filename": "EDI_AGG_OUTPATIENT_20240814_0252CDT.csv.gz", "event": "File to download", "logger": "delphi_doctor_visits.run", "level": "info", "pid": 68402, "timestamp": "2024-08-14T18:39:53.062878Z"}
{"filename": "EDI_AGG_OUTPATIENT_20240813_1452CDT.csv.gz", "percent": 0, "event": "Transfer in progress", "logger": "delphi_doctor_visits.run", "level": "info", "pid": 68402, "timestamp": "2024-08-14T18:39:53.182709Z"}
{"filename": "EDI_AGG_OUTPATIENT_20240813_1452CDT.csv.gz", "percent": 25, "event": "Transfer in progress", "logger": "delphi_doctor_visits.run", "level": "info", "pid": 68402, "timestamp": "2024-08-14T18:40:09.275755Z"}
{"filename": "EDI_AGG_OUTPATIENT_20240813_1452CDT.csv.gz", "percent": 50, "event": "Transfer in progress", "logger": "delphi_doctor_visits.run", "level": "info", "pid": 68402, "timestamp": "2024-08-14T18:40:19.720957Z"}
{"filename": "EDI_AGG_OUTPATIENT_20240813_1452CDT.csv.gz", "percent": 75, "event": "Transfer in progress", "logger": "delphi_doctor_visits.run", "level": "info", "pid": 68402, "timestamp": "2024-08-14T18:40:30.740461Z"}
{"filename": "EDI_AGG_OUTPATIENT_20240813_1452CDT.csv.gz", "percent": 100, "event": "Transfer in progress", "logger": "delphi_doctor_visits.run", "level": "info", "pid": 68402, "timestamp": "2024-08-14T18:40:41.629636Z"}
{"filename": "EDI_AGG_OUTPATIENT_20240814_0252CDT.csv.gz", "percent": 0, "event": "Transfer in progress", "logger": "delphi_doctor_visits.run", "level": "info", "pid": 68402, "timestamp": "2024-08-14T18:40:41.721008Z"}
{"filename": "EDI_AGG_OUTPATIENT_20240814_0252CDT.csv.gz", "percent": 25, "event": "Transfer in progress", "logger": "delphi_doctor_visits.run", "level": "info", "pid": 68402, "timestamp": "2024-08-14T18:40:53.522189Z"}
{"filename": "EDI_AGG_OUTPATIENT_20240814_0252CDT.csv.gz", "percent": 50, "event": "Transfer in progress", "logger": "delphi_doctor_visits.run", "level": "info", "pid": 68402, "timestamp": "2024-08-14T18:41:04.152777Z"}
{"filename": "EDI_AGG_OUTPATIENT_20240814_0252CDT.csv.gz", "percent": 75, "event": "Transfer in progress", "logger": "delphi_doctor_visits.run", "level": "info", "pid": 68402, "timestamp": "2024-08-14T18:41:16.296555Z"}
{"filename": "EDI_AGG_OUTPATIENT_20240814_0252CDT.csv.gz", "percent": 100, "event": "Transfer in progress", "logger": "delphi_doctor_visits.run", "level": "info", "pid": 68402, "timestamp": "2024-08-14T18:41:27.713898Z"}

Associated Issue(s)

  • Addresses #(2017)

Copy link
Contributor

@dshemetov dshemetov left a comment

Choose a reason for hiding this comment

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

Huh, do we really need progress logging? Download start and end seem like the most important bits of information to me, but I'm totally unfamiliar with this code. If we're committed to logging these, this seems fine to me, though it's going to be a bit subtle to understand that progress_chunks is being modified on every call after a few months away from this code. I thought a little about how to handle this with generators, but that doesn't seem much easier to read. I'd add a comment above the remove, something to the effect of "# Remove progress chunk, so it is not logged again".

@aysim319
Copy link
Contributor Author

aysim319 commented Aug 14, 2024

Huh, do we really need progress logging? Download start and end seem like the most important bits of information to me, but I'm totally unfamiliar with this code. If we're committed to logging these, this seems fine to me, though it's going to be a bit subtle to understand that progress_chunks is being modified on every call after a few months away from this code. I thought a little about how to handle this with generators, but that doesn't seem much easier to read. I'd add a comment above the remove, something to the effect of "# Remove progress chunk, so it is not logged again".

¯_(ツ)_/¯ honestly, I'm not sure; it's a nice to have I guess... If people don't case about having the progress; I agree of just having a start and a finish; I feel like it seems a waste to constantly pinging this call back; it's a simple function, so not much harm I guess.

But, I did it this way with the assumption of we would prefer to have the progress.
I tried to tinker around a way that would be less subtle, but since this is called as a callback, i couldn't think of a way to basically remember that the checkpoint was already logged;

Doing it exact percentage would basically only log 0 and 100 since the 25,50,75 percentage were a fraction. logging around with rounding didn't get much. And having a temp variable and comparing doesn't work (basically would need to be defined not locally, since the comparsion would never be equal)...

Copy link
Contributor

@melange396 melange396 left a comment

Choose a reason for hiding this comment

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

I like this solution! @dshemetov : youre right, we dont necessarily NEED progress to be logged, but since it was there already and this fix is clean and concise, i think it is worth keeping. We definitely needed to change the way it was logged before -- there were like 140 log messages for each landmark percent!

@aysim319 aysim319 merged commit 2184da7 into main Aug 15, 2024
16 checks passed
@aysim319 aysim319 deleted the 2017-turn-down-progress-msg branch August 15, 2024 15:41
minhkhul added a commit that referenced this pull request Sep 12, 2024
* doc: add logger examples to README and format

* deployment + staging details

* wrap lines

* Update _template_python/INDICATOR_DEV_GUIDE.md

Co-authored-by: David Weber <[email protected]>

* Update _template_python/INDICATOR_DEV_GUIDE.md

Co-authored-by: David Weber <[email protected]>

* Update _template_python/INDICATOR_DEV_GUIDE.md

Co-authored-by: David Weber <[email protected]>

* Update INDICATOR_DEV_GUIDE.md with mysql syntax

* Update INDICATOR_DEV_GUIDE.md reorder instruction on cron jobs

* switch from input_file to input_dir (#1995)

* adjust nssp max_age

* add pyproject.toml

* made changes based on suggestion

* update ci config

* separate new line

Co-authored-by: Dmitry Shemetov <[email protected]>

* fix: add explicit boto and moto dependencies to indicators

* trying to get package data to show

* installing dev package before testing in ci

* adding dependency

* move comment to right package

* Update _delphi_utils_python/pyproject.toml

Co-authored-by: Dmitry Shemetov <[email protected]>

* Revert "Clean up delphi_utils "

* refactoring date parsing to seperate file

* first implimentation

* small cleanup and adding missing data

* changed wording and some logic

* made test more explicit

* add missing file

* cleaning up tests

* consolidating tests into one

* fixed wording

* consolidating test/fixture

* clean up test

* Update google_symptoms/delphi_google_symptoms/patch.py

remove old code; not used anymore

Co-authored-by: nmdefries <[email protected]>

* clean up lint

* remove teardown

* cleaning and renaming

* more explicit branching for regular run vs patching, renaming, cleaning

* cleanup and rewrite logic

* avoid linking keywords in PR template

* Update _delphi_utils_python/README.md

Co-authored-by: george <[email protected]>

* Refactor Jenkins pipeline

- Remove usage of parallel in the build and package stages
- Retain usage of parallel in the deploy stages
- Separate build and package into separate scripts to support build-only stages
- Add a build-only stage for dev/feature branches

* Add separate build and package scripts

* Update script comments

* Remove unused script

* changes based on suggestion

* standardizing output dir in test

* changing back to session scope

* more logging and update params

* adding back conditional for num_export_days

* test_run does not like fixtures

* make deep copies of params fixtures

* linting

* test patch check if num_export_days is set or not

* patch fn takes params as an arg, like run_module

* list values added using actual day as an example

* move pull date creation into pull_gs_data

* don't modify num_export_days; define start/end date in terms of it and lag

* refer to params by name; patch_flag -> custom_run flag

* use test data based on recent actual indicator run

* say where gold test data came from

* not all date_utils tests need metadata

* formatting

* fn docs

* lint and cleanup

* adding more tests

* removed extra lag and made test more robust

* export_start_date is optional since the first available date is baked in

* need to account for historical / current run for adding lag

* adding validation script

* fix typo

* fixing typo in test

* lint

* sample run for validation

* remove unused input

Co-authored-by: nmdefries <[email protected]>

* remove unused import

* added progress chunk to limit logging (#2024)

* added progress chunk to limit logging

* lint

* comment based on suggestion

* add hhs geo level (#2036)

* Add discussion of patching functionality to indicator manual (#2031)

* basic patching info

* custom_run

* add hhs to geo section

* de-emphasize R in contributing doc, and add links

* ref/issue date intro

* naming and force push standards

* secret and params links

* Clean up delphi_utils (take 2) (#2014)

* add pyproject.toml

* made changes based on suggestion

* update ci config

* separate new line

Co-authored-by: Dmitry Shemetov <[email protected]>

* fix: add explicit boto and moto dependencies to indicators

* trying to get package data to show

* installing dev package before testing in ci

* adding dependency

* move comment to right package

* Update _delphi_utils_python/pyproject.toml

Co-authored-by: Dmitry Shemetov <[email protected]>

* more suggestion

* adding pyproject.toml to template

* Update changehc/Makefile

* Update claims_hosp/Makefile

* Update doctor_visits/Makefile

* Update google_symptoms/Makefile

* Update hhs_hosp/Makefile

* Update nchs_mortality/Makefile

* Update nssp/Makefile

* Update nwss_wastewater/Makefile

* Update quidel_covidtest/Makefile

* Update sir_complainsalot/Makefile

* fix: dependence bugs and cleanup
* pandas[excel]<2 doesn't work, go back to xlrd and comment
* add mock to changehc
* add requests
* remove setup.py from template

* build: fix Jenkins build script

---------

Co-authored-by: Amaris Sim <[email protected]>
Co-authored-by: aysim319 <[email protected]>
Co-authored-by: Dmitry Shemetov <[email protected]>

* fix: build-indicators.sh looks for setup.py and pyproject.toml (#2048)

* fix: build-indicators.sh looks for setup.py again

Reverting a premature change.

* fix: readability and future-proof

* fix: more readability

* chore: bump delphi_utils to 0.3.25

* chore: bump covidcast-indicators to 0.3.56

* [create-pull-request] automated change

---------

Co-authored-by: Dmitry Shemetov <[email protected]>
Co-authored-by: minhkhul <[email protected]>
Co-authored-by: Nat DeFries <[email protected]>
Co-authored-by: minhkhul <[email protected]>
Co-authored-by: David Weber <[email protected]>
Co-authored-by: george <[email protected]>
Co-authored-by: Amaris Sim <[email protected]>
Co-authored-by: aysim319 <[email protected]>
Co-authored-by: Brian Clark <[email protected]>
Co-authored-by: Delphi Deploy Bot <[email protected]>
Co-authored-by: minhkhul <[email protected]>
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.

3 participants