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

Better Instrumentation #265

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft

Conversation

pepoluan
Copy link
Collaborator

@pepoluan pepoluan commented Mar 11, 2021

What do these changes do?

  • Improve existing instrumentation by using LoggingAdapter, to make code maintainability better.
  • Add more logging points to help troubleshooting
  • Also implement an "exception handler" for Controller to (1) reduce incidents of "Exception ignored", and (2) help troubleshoot

Are there changes in behavior for the user?

Users relying on log entries will need to adapt to the newer, more detailed log entries.

Related issue number

None

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • tox testenvs have been executed in the following environments:
    • Windows 10 (via PyCharm tox runner)
    • Windows 10 (via PSCore 7.1.2)
    • Windows 10 (via Cygwin)
    • Ubuntu 18.04 on WSL 1.0
    • FreeBSD 12.2 on VBox
    • OpenSUSE Leap 15.2 on VBox
  • Documentation reflects the changes
  • Add a news fragment into the NEWS.rst file

Comment on lines +58 to +67
class ContextLoggerAdapter(logging.LoggerAdapter):
@property
def context(self):
return self.extra.get("context")

def process(
self, msg: Any, kwargs: MutableMapping[str, Any]
) -> Tuple[Any, MutableMapping[str, Any]]:
msg = f"[{self.context}] {msg}" if self.context else msg
return msg, kwargs
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

LoggerAdapter is awesome! Why haven't I known about this before...

Comment on lines +183 to +189
class BaseControllerMapping:
__slots__ = ()

def __get__(self, instance: "BaseController", owner):
return {
"context": instance.name,
}
Copy link
Collaborator Author

@pepoluan pepoluan Mar 11, 2021

Choose a reason for hiding this comment

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

Descriptors are awesome! Perfect for what I need to implement the LoggerAdapters 😎

Comment on lines +135 to +140
@pytest.fixture(autouse=True)
def log_case(request: pytest.FixtureRequest):
node_id = request.node.nodeid
log.debug("Entering %s", node_id)
yield
log.debug("Exiting %s", node_id)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This makes test logs easier to read since we now see which test case triggered which sequence of ops.

Comment on lines -539 to +541
log.debug("Waiting for PROXY signature")
signature = await reader_func.readexactly(5)
try:
log.debug("Waiting for PROXY signature")
signature = await reader_func.readexactly(5)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure why I left these 2 lines outside of try..except...

@codecov
Copy link

codecov bot commented Mar 11, 2021

Codecov Report

Merging #265 (29e1b7c) into master (747a7c4) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            master      #265    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files            7         7            
  Lines         1656      1763   +107     
  Branches       306       319    +13     
==========================================
+ Hits          1656      1763   +107     
Impacted Files Coverage Δ
aiosmtpd/__init__.py 100.00% <100.00%> (ø)
aiosmtpd/controller.py 100.00% <100.00%> (ø)
aiosmtpd/main.py 100.00% <100.00%> (ø)
aiosmtpd/proxy_protocol.py 100.00% <100.00%> (ø)
aiosmtpd/smtp.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 747a7c4...29e1b7c. Read the comment docs.

@pepoluan pepoluan added this to the 1.5 milestone Mar 12, 2021
@pepoluan
Copy link
Collaborator Author

Sorry for the radio silence. I had been tearing my hair out trying to find where ResourceWarnings happen...

@pepoluan pepoluan mentioned this pull request Mar 22, 2021
11 tasks
@waynew
Copy link
Collaborator

waynew commented Nov 25, 2022

@pepoluan Hey! So I'm finally devoting some time back to aiosmtpd, not sure what your next couple of weeks look like 😂

Do you think that this should be a blocker for the 1.5 release (or do you think that you'll be able to get it un-draft-ed in the next week or two)?

Glancing through it again, I'm 👍 on what I see, generally, though I do want to take a deeper dive. I don't think I'll get to it at all this weekend, but I'm going to aim at digging into it some evening this coming week -- unless you'd rather just wait on merging it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants