Skip to content

ENH: log exception filename/line number#65

Merged
klauer merged 1 commit into
pcdshub:masterfrom
klauer:enh_log_exc_filename
Jul 14, 2022
Merged

ENH: log exception filename/line number#65
klauer merged 1 commit into
pcdshub:masterfrom
klauer:enh_log_exc_filename

Conversation

@klauer
Copy link
Copy Markdown
Contributor

@klauer klauer commented Jul 13, 2022

Description

Add exc_filename and exc_line to global log JSON.

Motivation and Context

Closes #63

How Has This Been Tested?

  • Interactively with a script that raises in the main thread and a background thread
  • In hutch-python

Where Has This Been Documented?

Issue and PR

Screenshots (if appropriate):

image

Related but not dependent: pcdshub/hutch-python#346

Comment thread pcdsutils/log.py
import types
import typing
import warnings
from typing import Optional, Tuple, Union
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a reason we take out the typing. for some of the type hints but not all of them? Are hints like TextIO considered not well-known enough to omit the parent package?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

A good reason? No. I tend to go back and forth and annoy reviewers with this.

  1. I like to import the common ones by name Tuple, Union, List are used everywhere (to reduce typing)
  2. Uncommon ones or those that get used perhaps once like TextIO I tend to do with the qualified name - i.e., typing.TextIO

The stylistic issue of which to choose will go away at some point when we are on 3.9+ everywhere (since we can do list[int] instead of typing.List[int])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I typically just from typing import x for all of them.

In general I'm pretty happy with using from imports for everything except when the context is important to understanding the code. For example, if there was a common namespace collision I'd probably avoid the from import...

It's not very important in most cases I think.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I also avoid from imports when the function name is super generic... like post or read or something like that. modulename.post is much clearer in the code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Other random ones, not talking about typing.X specifically:

  • It's easier to monkeypatch at a module level when using module.obj
  • It's easier to grep for usage when you have module.obj

In general I think import module; module.obj is superior in most ways, except when used excessively that it makes the code unnecessarily long.

Copy link
Copy Markdown
Contributor

@tangkong tangkong left a comment

Choose a reason for hiding this comment

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

Adding some extra fields to the json, looks simple enough. Shows up in grafana too 👍

Copy link
Copy Markdown
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

This looks good to me

@klauer klauer merged commit a0497ad into pcdshub:master Jul 14, 2022
@klauer klauer deleted the enh_log_exc_filename branch July 14, 2022 17:52
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.

Centralized logger: additional key for exception source filename/line

3 participants