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

remove MultilineLogger, split logs with \n into multiple messages #1377

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

braingram
Copy link
Collaborator

@braingram braingram commented Aug 22, 2024

I marked this PR as no-changelog-entry-needed as I don't believe it changes any public API.

The use of MultilineLogger is problematic as it causes all later created loggers to use this class (which is not 100% compatible with all logging usage). As the main thing it achieves is splitting log messages that contain newlines into multiple lines this PR also switches all log messages (that I was able to find) that contain a newline into multiple log messages. Note that since MultilineLogger is registered during an import it is unpredictable which loggers become instances of MultilineLogger (and which are the default Logger) so it's likely this PR introduces a small change in the log output for some modified log messages.

Although unknown at this point, MultilneLogger may be contributing to the issues currently seen with the metrics_logger (see https://jira.stsci.edu/browse/SCSB-174)

Regression tests running at https://github.com/spacetelescope/RegressionTests/actions/runs/10514305515

Checklist

  • added entry in CHANGES.rst under the corresponding subsection
  • updated relevant tests
  • updated relevant documentation
  • updated relevant milestone(s)
  • added relevant label(s)
  • ran regression tests, post a link to the Jenkins job below. How to run regression tests on a PR

@braingram braingram marked this pull request as ready for review August 22, 2024 19:20
@braingram braingram requested a review from a team as a code owner August 22, 2024 19:20
@braingram braingram changed the title remove MultilineLogger split logs with \n into multiple messages remove MultilineLogger, split logs with \n into multiple messages Aug 22, 2024
Copy link

codecov bot commented Aug 22, 2024

Codecov Report

Attention: Patch coverage is 16.66667% with 5 lines in your changes missing coverage. Please review.

Project coverage is 78.66%. Comparing base (8f72709) to head (a20997e).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
romancal/flux/flux_step.py 0.00% 3 Missing ⚠️
romancal/associations/association.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1377      +/-   ##
==========================================
- Coverage   78.71%   78.66%   -0.06%     
==========================================
  Files         117      117              
  Lines        7738     7724      -14     
==========================================
- Hits         6091     6076      -15     
- Misses       1647     1648       +1     
Flag Coverage Δ *Carryforward flag
nightly 62.12% <ø> (-0.14%) ⬇️ Carriedforward from 8f72709

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ddavis-stsci
Copy link
Collaborator

We should ask @stscieisenhamer what problem the MultilineLogger was put in to solve. Since it is in the associations I'm assuming that he will know.

@ddavis-stsci
Copy link
Collaborator

removing the MultilineLogger package from the associations results in all 29 DMS test numbers in the log file for the regression tests.
All the unit tests passed and I see the regression tests pass so I think this change is good to go.

@braingram
Copy link
Collaborator Author

braingram commented Aug 23, 2024

Thanks for testing this. Which log messages are you referring to? Specifically, what's the log message for DMS413?

@ddavis-stsci
Copy link
Collaborator

2186:2024-08-23 10:56:13,773 - stpipe.ExposurePipeline - INFO - DMS413 MSG: Testing that result is a Level 2 model.......Pass
2187:[stpipe.ExposurePipeline] DMS413 MSG: Testing that result is a Level 2 model.......Pass
2188:2024-08-23 10:56:13,773 - stpipe.ExposurePipeline - INFO - DMS413 MSG: Testing that there are 16 resultants in the input file.......Pass
2189:[stpipe.ExposurePipeline] DMS413 MSG: Testing that there are 16 resultants in the input file.......Pass
2190:2024-08-23 10:56:13,773 - stpipe.ExposurePipeline - INFO - DMS413 MSG: Testing that there are 16 resultants listed in the output file.......Pass
2191:[stpipe.ExposurePipeline] DMS413 MSG: Testing that there are 16 resultants listed in the output file.......Pass
2192:[metrics_logger] METRIC - DMS413 - test_16resultants_image_processing - PASS
(

@braingram
Copy link
Collaborator Author

Thanks! I was recently looking at the code that parses this and I believe only the 2192:[metrics_logger] METRIC - DMS413 - test_16resultants_image_processing - PASS line "works" for the parser. Is that your understanding as well?

@braingram braingram force-pushed the cleanup_log_newlines branch from 8fe7bfb to b4e01a1 Compare August 23, 2024 19:23
@ddavis-stsci
Copy link
Collaborator

Thanks! I was recently looking at the code that parses this and I believe only the 2192:[metrics_logger] METRIC - DMS413 - test_16resultants_image_processing - PASS line "works" for the parser. Is that your understanding as well?

I have not really looked at the metrics logger code. I can if needed...

@braingram
Copy link
Collaborator Author

I have not really looked at the metrics logger code. I can if needed...

Thanks for the offer. Hopefully that will not be needed. I don't think this PR will fully fix the log parsing issues but I think it's more a sign that using the logs for testing DMS requirements is problematic (given the state of logging in the pipeline). @zacharyburnett has an alternative solution in the works so hopefully things will change (for the better) soon.

@braingram braingram force-pushed the cleanup_log_newlines branch from ed7131c to a20997e Compare October 1, 2024 13:48
Copy link
Collaborator

@stscieisenhamer stscieisenhamer left a comment

Choose a reason for hiding this comment

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

It was a solution to making messages all have a logging tag. If it is a problem, then it can be removed.

@braingram braingram merged commit b652ef8 into spacetelescope:main Oct 1, 2024
29 of 31 checks passed
@nden nden added this to the 25Q1_B16 milestone Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants