Skip to content

DM-33931: Improve logging #9

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Mar 11, 2022
Merged

DM-33931: Improve logging #9

merged 6 commits into from
Mar 11, 2022

Conversation

kfindeisen
Copy link
Member

@kfindeisen kfindeisen commented Mar 10, 2022 •
edited
Loading

This PR cleans up the logging to make it more user-friendly.

@kfindeisen kfindeisen marked this pull request as draft March 10, 2022 20:42
timj
timj reviewed Mar 10, 2022
View reviewed changes
python/activator/activator.py
def check_for_snap(
instrument: str, group: int, snap: int, detector: int
) -> Optional[str]:
prefix = f"{instrument}/{detector}/{group}/{snap}/{instrument}-{group}-{snap}-"
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to make use of the MDC feature that we use in butler logging? That lets you set an MDC record and after that every log message will get that content included without you having to explicitly add "prefix" to every log message.

@kfindeisen kfindeisen requested a review from parejkoj March 11, 2022 00:12
@kfindeisen kfindeisen marked this pull request as ready for review March 11, 2022 00:12
parejkoj
parejkoj reviewed Mar 11, 2022
View reviewed changes
Copy link
Contributor

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

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

Thanks for this. One comment about how we name the loggers (and possibly one comment that I can't see any more about f-strings).

Thanks for leaving the f-strings: let's see where the discussion on that ends up before we convert them all.

@kfindeisen kfindeisen merged commit 73567eb into main Mar 11, 2022
@kfindeisen kfindeisen deleted the tickets/DM-33931 branch March 11, 2022 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timj timj timj left review comments

@parejkoj parejkoj parejkoj left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@kfindeisen @timj @parejkoj