-
Notifications
You must be signed in to change notification settings - Fork 361
Add opt-in per-instrument logger scope (default_logger_scope) #8523
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
Open
ymampaey
wants to merge
6
commits into
microsoft:main
Choose a base branch
from
ymampaey:feature/scoped-logger
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
c5e1c8e
implement scoped logger
ymampaey 4904ea8
add newsfragment
ymampaey 944512b
Rename newsfragment to match PR number
ymampaey d2581a9
Document logging registry growth caveat in the logging example
ymampaey 14e13a7
Clarify what propagate = False does in the logging example
ymampaey ea454fa
add ClassVar LoggerScope
ymampaey File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| Instruments can now opt in to a logger of their own, rather than sharing a | ||
| single logger with every other instrument in the process. Setting the class | ||
| attribute ``default_logger_scope`` to ``"instrument"`` makes ``Instrument.log`` | ||
| (and ``VisaInstrument.visa_log``) use a logger named after the driver class and | ||
| the instrument's ``name_parts``, so that levels, handlers and ``propagate`` can | ||
| be configured per instrument:: | ||
|
|
||
| class MyDriver(VisaInstrument): | ||
| default_logger_scope = "instrument" | ||
|
|
||
| The scoped loggers mirror the instrument hierarchy, for example | ||
| ``qcodes.instrument.instrument_base.MyDriver`` for the driver, | ||
| ``qcodes.instrument.instrument_base.MyDriver.myinst`` for one of its | ||
| instruments and ``qcodes.instrument.instrument_base.MyDriver.myinst.ChanA`` for | ||
| one of that instrument's channels. A level can therefore be set for a whole | ||
| driver class, a single instrument or a single channel, and is inherited by | ||
| everything below it. Setting the level for a whole driver also works before any | ||
| of its instruments exist, so it can be configured via ``logger_levels`` in | ||
| ``qcodesrc.json``. | ||
|
|
||
| The default is unchanged: without opting in, all instruments keep sharing one | ||
| logger exactly as before. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would basically be equivalent to replacing base which is currently the name of this module
qcodes.instrument.instrument_basewith the name of the module that the instrument is defined in.qcodes.insrument.instrument_vendor.instrument_filename. I think I agreee that I would prefer thisThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One blocker for replacing base with the name of the module the instrument is defined in: _logger_name is called with two different bases, name for self.log and VISA_LOGGER for self.visa_log. Drop base, and those two become the same logger name, so driver messages and wire traffic can no longer be separated at all. Module-only also wouldn't distinguish AMIModel430 from AMIModel4303D, since both live in AMI430_visa.py.
The problem is:
Not sure if vendors will realistically have the same module number, but perhaps these other sources could cause a problem:
In the category "same lineage, different module". Copilot's finding is technically correct but low severity: it needs a self-inflicted name clash, and the consequence is a shared log level, not data loss or a crash.
We could do the following:
So it would look like:
Which is safer but very verbose. Another alternative is leaving it as is and documenting the behavior.