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

Improve analysis logging #578

Merged
merged 13 commits into from
Mar 22, 2023
Merged

Improve analysis logging #578

merged 13 commits into from
Mar 22, 2023

Conversation

cortadocodes
Copy link
Member

@cortadocodes cortadocodes commented Mar 22, 2023

Summary

This PR refines the approach taken to adding metadata about an analysis to log messages emitted during its processing. It fixes an issue where developers couldn't test their apps based on the log messages emitted during an analysis run.

Contents (#578)

New features

  • Enable Octue log handler by default

Enhancements

  • Avoid using deprecated datafile property in runner
  • Add default Octue service Dockerfile that uses python3.10

Fixes

  • Swap out formatters instead of handlers during analyses
  • Only use Octue formatter in analysis if envvar present

Refactoring

  • Rename AnalysisLogHandlerSwitcher to AnalysisLogFormatterSwitcher
  • Move run_logged_subprocess to octue.utils.processes

Testing

  • Add tests for AnalysisLogFormatterSwitcher

@cortadocodes cortadocodes self-assigned this Mar 22, 2023
@cortadocodes cortadocodes linked an issue Mar 22, 2023 that may be closed by this pull request
5 tasks
@thclark
Copy link
Contributor

thclark commented Mar 22, 2023

@cortadocodes does this deprecate USE_OCTUE_LOG_HANDLERS in favour of USE_OCTUE_LOG_FORMATTERS?

@cortadocodes
Copy link
Member Author

@cortadocodes does this deprecate USE_OCTUE_LOG_HANDLERS in favour of USE_OCTUE_LOG_FORMATTERS?

Not yet. I need to think about which of the two names is more appropriate because, if the environment variable is "1", a handler is applied to the root logger and the pub/sub handler is applied during analyses.

@codecov-commenter
Copy link

Codecov Report

Merging #578 (bee1b9d) into main (a87dd3d) will increase coverage by 0.75%.
The diff coverage is 94.60%.

@@            Coverage Diff             @@
##             main     #578      +/-   ##
==========================================
+ Coverage   93.99%   94.74%   +0.75%     
==========================================
  Files          75       79       +4     
  Lines        3115     3484     +369     
==========================================
+ Hits         2928     3301     +373     
+ Misses        187      183       -4     
Impacted Files Coverage Δ
octue/cloud/deployment/google/dataflow/setup.py 0.00% <ø> (ø)
octue/essentials/monitor_messages.py 100.00% <ø> (ø)
octue/cli.py 86.48% <80.64%> (-2.41%) ⬇️
octue/app_loading.py 84.00% <84.00%> (ø)
octue/cloud/pub_sub/topic.py 94.44% <85.00%> (+0.96%) ⬆️
octue/cloud/pub_sub/subscription.py 91.54% <87.50%> (+4.04%) ⬆️
octue/resources/datafile.py 93.98% <88.23%> (-0.15%) ⬇️
octue/utils/testing.py 90.90% <90.90%> (ø)
octue/cloud/pub_sub/service.py 93.47% <92.20%> (+2.42%) ⬆️
octue/cloud/deployment/google/base_deployer.py 95.04% <92.85%> (+0.36%) ⬆️
... and 26 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@cortadocodes cortadocodes merged commit 55eadcc into main Mar 22, 2023
@cortadocodes cortadocodes deleted the fix/analysis-logging branch March 22, 2023 20:03
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.

Adjust analysis log handling to not remove current log handlers
3 participants