Skip to content

Add centralized logger. #1031

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

Merged
merged 17 commits into from
Jun 13, 2022
Merged

Conversation

jverswijver
Copy link
Contributor

@jverswijver jverswijver commented Jun 2, 2022

Adds a package level logger for the datajoint package.

Should accomplish the following:

  • bring all the loggers under the same logger, in the current DJ we make loggers per module.
  • convert all warnings to logs with level WARNING
  • update any tests
  • convert any applicable print statements to logs
    - [ ] Evaluate log DB table to see if logging needs to be added

Additional functionality requested by data science team:

- [ ] log warnings when user calls populate on empty key_source
- [ ] debug level logging for any insert/delete

  • example for adding handler to the logger so they can send log output to sources other than stdout

Example for capturing the log output but maintaining dj logging

See the converted test test_limit_warning in the PR.
Basically you add a handler to the logger that sends all logs to a StringIO object and then retrieve the string from the object, then you remove the handler.

@guzman-raphael guzman-raphael linked an issue Jun 2, 2022 that may be closed by this pull request
@ttngu207
Copy link
Contributor

ttngu207 commented Jun 9, 2022

Suggesting the logging related to transaction to be set at DEBUG level

logger.info("Transaction started")

logger.info("Transaction cancelled. Rolling back ...")

logger.info("Transaction committed and closed.")

@jverswijver
Copy link
Contributor Author

jverswijver commented Jun 10, 2022

Yes, this makes sense I will do that. @ttngu207

Copy link
Collaborator

@guzman-raphael guzman-raphael left a comment

Choose a reason for hiding this comment

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

@jverswijver Great work! 💪

Mostly ready just offering further improvements.

Copy link
Collaborator

@guzman-raphael guzman-raphael left a comment

Choose a reason for hiding this comment

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

@jverswijver Nice work! 🤝

@@ -288,6 +288,9 @@ def _populate1(
exception=error.__class__.__name__,
msg=": " + str(error) if str(error) else "",
)
logger.debug(
f"Error making {key} -> {self.target.table_name} - {error_message}"
Copy link
Member

Choose a reason for hiding this comment

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

I think table_name does not include the schema name whereas full_table_name does.

Suggested change
f"Error making {key} -> {self.target.table_name} - {error_message}"
f"Error making {key} -> {self.target.full_table_name} - {error_message}"

@guzman-raphael guzman-raphael merged commit 8f9e81c into datajoint:master Jun 13, 2022
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.

Mix of print statements and logging results in messy logs
4 participants