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 logging #51

Merged
merged 5 commits into from
Nov 12, 2024
Merged

Add logging #51

merged 5 commits into from
Nov 12, 2024

Conversation

Tom-Willemsen
Copy link
Contributor

@Tom-Willemsen Tom-Willemsen commented Nov 6, 2024

Closes #45

  • Adds logging throughout the library
  • Adjusts some of the logging setup code as it was accidentally capturing messages from root logger and not quite adhering to normal logging idioms in some places. Also made easier to adjust log levels on the fly in e.g. an interactive session.

To review:

  • Check logging statements generally make sense
  • Run a representative scan with INFO logging (default), check you get a reasonable set of messages in the log file.
    • Reasonable = not too many, not too few - tricky balance but should generally be logging the "main" things that happen during a scan.
  • Run the same scan with DEBUG logging on all of bluesky/ibex_bluesky_core/ophyd_async (see added docs for how to do this), check that you get much more detailed debug logging which would help to diagnose tricky low-level issues.
  • Check you don't pick up messages from other loggers (e.g. run logging.getLogger("foo").warn("hello") in pydev console, check that doesn't end up in the bluesky log.
  • Check tests make sense etc

ibex_bluesky_core needs to be released after merging this so that system tests pick up the changes.

@Tom-Willemsen Tom-Willemsen force-pushed the add_logging branch 3 times, most recently from 08a87c6 to 8d692f2 Compare November 6, 2024 22:44
And refactor existing logging code to be more standard
@Tom-Willemsen Tom-Willemsen marked this pull request as ready for review November 7, 2024 15:01
Chsudeepta
Chsudeepta previously approved these changes Nov 12, 2024
@Chsudeepta Chsudeepta self-requested a review November 12, 2024 13:19
@Chsudeepta Chsudeepta assigned Chsudeepta and unassigned Chsudeepta Nov 12, 2024
@Chsudeepta Chsudeepta removed their request for review November 12, 2024 13:21
@Chsudeepta
Copy link
Contributor

I have completed the review however merging is blocked as the last push was done by me. It now requires a different reviewer to complete the merging business

@Tom-Willemsen Tom-Willemsen requested a review from rerpha November 12, 2024 13:25

__all__ = ["setup_logging", "set_bluesky_log_levels", "file_handler"]

DEFAULT_LOG_FOLDER = os.path.join("C:\\", "instrument", "var", "logs", "bluesky")
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: could use a Pathlib Path() here

doc/dev/logging.md Outdated Show resolved Hide resolved
Co-authored-by: Jack Harper <[email protected]>
@Chsudeepta Chsudeepta merged commit ea13a3c into main Nov 12, 2024
11 checks passed
@Chsudeepta Chsudeepta deleted the add_logging branch November 12, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add useful logging for Bluesky
3 participants