12188 add logging handler context managers #12207

glyph · 2024-06-09T00:47:24Z

This is a retread of #12196 with a simpler commit history. Its description:

Scope and purpose

Fixes #12188

Fixes #12195

This includes all the changes from #12189 , but attempts to demonstrate their utility. In so doing, I discovered a performance regression due to the additional allocation from creating a new context manager, so this also adds Logger.failureHandler, which allows for pre-allocating a context manager with a fixed error message, suitable for use within hot loops.

It also includes a ton of examples of the usages of both decorators, as well as a huge amount of refactoring, because:

many of the places that I wanted to use this are very old Twisted code

ThreadedSelectReactor was, in particular, a buggy mess with no test coverage that basically didn't work

using this new API requires migrating from old-logger to new-logger, so I had to go do that too

as I migrated I wanted type checking, and touching type checking in some places caused knock-on changes to be needed

in addressing the coverage gaps, I fixed a few bugs, in particular in half-close support, which previously just … did not work at all, with StandardIO just never calling writeConnectionLost at all.

There are no particularly interesting bugs fixed here, or major new subsystems with full type coverage, so not much for the release notes, but i think it's worth dealing with a slightly larger and more diffuse PR for getting all these minor improvements.

…t reactor

codspeed-hq · 2024-06-09T00:49:01Z

CodSpeed Performance Report

Merging #12207 will not alter performance

_{Comparing 12188-redux (8f0643c) with trunk (c99802c)}

Summary

✅ 2 untouched benchmarks

glyph · 2024-06-09T00:57:33Z

@twm fyi this is your fault for making me actually consider the design

(Thank you, the code is much better for it, but still, there's like 1000 lines now)

glyph · 2024-06-09T01:11:32Z

Sigh, trying one more force push to get rid of spurious codecov statuses that seem to have gotten stuck to the win32 stdio changes.

adiroiban · 2024-06-10T09:37:44Z

Sigh, trying one more force push to get rid of spurious codecov statuses that seem to have gotten stuck to the win32 stdio changes.

Not sure if you are aware of the "Re-run failed jobs" feature.

Maybe a force push is easier for your, so that's also an option.

docs/core/howto/logger.rst

src/twisted/internet/base.py

adiroiban · 2024-06-10T09:54:20Z

docs/core/howto/logger.rst


-:py:class:`Logger <twisted.logger.Logger>` provides a :py:meth:`failure <twisted.logger.Logger.failure>` method, which allows one to capture a :py:class:`Failure <twisted.python.failure.Failure>` object conveniently:
+:py:class:`Logger <twisted.logger.Logger>` provides a :py:meth:`failure <twisted.logger.Logger.handlingFailures>` method, which allows one to run some application code, then capture a :py:class:`Failure <twisted.python.failure.Failure>` in the log stream if that code raises an exception:


Maybe we should also have an example demonstrating the usage of the "Operation" ... or at least a few words about the fact that the context manager returns an "operation" that can be used to check the result and contains a reference to the raised Failure

adiroiban

Thanks for the update.

I did a quick review and left a few comments.

I haven't looked into the details of win32 and threadselect changes

I think that this is a big improvement.

I have approved it, but I am leaving this for review for Tom or other team members.

If a further review is delayed, I think that this can be merged.

Thanks again!

glyph · 2024-06-10T21:57:41Z

Thanks @adiroiban, I really appreciate that! I think this is one that might benefit from some post-hoc review, so after reviewing and addressing your comments I might land pretty quickly, but will be interested in feedback from others later.

glyph · 2024-06-10T22:00:06Z

Not sure if you are aware of the "Re-run failed jobs" feature.

Maybe a force push is easier for your, so that's also an option.

The problem in this case is that the job did not fail, but that codecov reported annotations

Not sure if you are aware of the "Re-run failed jobs" feature.

Maybe a force push is easier for your, so that's also an option.

I use it all the time :)

In this particular case, the issue is that the job did not actually fail. Codecov ran, it emitted some annotations, failed the check, then withdrew the failure once the Windows coverage arrived, but didn't withdraw the annotations. You can see them on the current files changed tab like this:

which of course makes no sense given that the patch is 100% covered and these are patch lines. I was hoping that the bug was intermittent and might be addressed by a full rebuild, but no, it looks like a flaw in codecov.

glyph · 2024-06-10T22:13:38Z

I have approved it

huh, why did the robot not notice this and add needs-merge ?

adiroiban · 2024-06-20T21:57:51Z

It might be due to this

glyph requested a review from twm 2 weeks ago

I think that as long as there are requested reviewers, it is not yet considered ready for merge.

glyph · 2024-06-20T21:59:15Z

I think that as long as there are requested reviewers, it is not yet considered ready for merge.

OK, I can remove twm then

glyph · 2024-06-20T22:00:18Z

Oh… but that doesn't help, because… somehow you reviewed as you, and not "on behalf of twisted-contributors"? Hmmm. I guess I should just mention people and never use the review-request mechanism.

glyph · 2024-06-20T22:24:29Z

siiiiiigh https://www.dockerstatus.com/pages/incident/533c6539221ae15e3f000031/6674ab18f483b5178cf6bdab

glyph · 2024-06-20T22:42:27Z

Not going to wait for Docker to resolve their incident here, but if there's a problem we can revert.

adiroiban · 2024-06-21T00:22:10Z

Oh… but that doesn't help, because… somehow you reviewed as you, and not "on behalf of twisted-contributors"? Hmmm. I guess I should just mention people and never use the review-request mechanism.

The review robot is not AI.
It has a fixed set of rules, defined at https://github.com/chevah/github-hooks-server/blob/main/chevah/github_hooks_server/handler.py

The main use case for the robot is to allow non Twisted Org members to manage add the review label

glyph · 2024-06-21T00:43:36Z

The main use case for the robot is to allow non Twisted Org members to manage add the review label

Yeah, I should contribute back to it a bit more :). But I really want in-org members to use it as much as possible, so we have a common workflow.

adiroiban · 2024-06-21T11:32:26Z

Yeah, I should contribute back to it a bit more :)

You can start by submitting bug reports and defining the rules/requirements/specifications for the robot.

I think that the robot works.

For this PR, you have explicitly requested a review from Tom and Tom has not reviewed the PR yet.
So I think that the PR should not be considered ready to merge.

For example see #12213

The PR was started as draft. As soon as the PR was no longer a draft the review label was added and a review was requested for the whole team.

I have reviewed the change and the PR was marked for merge.

Another example is #12216 ... I have expliclty requested the review, the label was autoatically applied and the Twisted team was automatically added to review.

As soon as someone from the team has reviewd and approved the PR, it was marked for merge.

Another example is #12202 ... marked as no longer a draft, the review was automatically request, I have requested changes and the label was automatically updated.

Twisted recently changed behavior of logger on failures [1]. It newly logs the `Main loop terminated.` even on exceptions, which breaks two test in twistedsupport test suite. This hack attempts to address the upcoming issue. [1] twisted/twisted#12207

glyph added 6 commits June 8, 2024 17:42

add Logger.handlingFailures and Logger.failureHandler

cece121

documentation

e3e21d4

use new logging in twisted.web

98590ae

use new log failure handling in stdio (and fix a few bugs)

d72b67e

use new failure handlers in reactor cores, and rewrite _threadedselec…

1269e56

…t reactor

move _sslverify to new logging

197f3c7

This was referenced Jun 9, 2024

12195 some examples #12196

Closed

add Logger.handlingFailures method #12189

Closed

glyph marked this pull request as ready for review June 9, 2024 00:56

chevah-robot added the needs-review label Jun 9, 2024

chevah-robot requested a review from a team June 9, 2024 00:57

glyph requested a review from twm June 9, 2024 00:57

move AMP to twisted.logger and adopt Logger.failure

dca0e46

glyph force-pushed the 12188-redux branch from 18e1328 to dca0e46 Compare June 9, 2024 01:11

adiroiban reviewed Jun 10, 2024

View reviewed changes

docs/core/howto/logger.rst Outdated Show resolved Hide resolved

adiroiban reviewed Jun 10, 2024

View reviewed changes

src/twisted/internet/base.py Show resolved Hide resolved

adiroiban reviewed Jun 10, 2024

View reviewed changes

adiroiban approved these changes Jun 10, 2024

View reviewed changes

adiroiban requested a review from a team June 10, 2024 09:55

glyph added 4 commits June 20, 2024 13:45

Merge remote-tracking branch 'origin/trunk' into 12188-redux

52ac0e6

rename .handlingFailures() to .failuresHandled()

0d903d4

address adi's feedback re: describing failure handlers

d85e4bd

Operation is public API, so give it docstrings.

8f42364

add docs for failureHandler and document Operation

8f0643c

glyph removed the request for review from twm June 20, 2024 21:59

glyph force-pushed the 12188-redux branch from f59ec66 to 8f0643c Compare June 20, 2024 22:23

glyph merged commit 8487b2b into trunk Jun 20, 2024
43 of 47 checks passed

glyph deleted the 12188-redux branch June 20, 2024 22:42

arkamar mentioned this pull request Jul 1, 2024

Prepare tests for upcoming twisted version testing-cabal/testtools#387

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

12188 add logging handler context managers #12207

12188 add logging handler context managers #12207

glyph commented Jun 9, 2024

codspeed-hq bot commented Jun 9, 2024 •

edited

Loading

glyph commented Jun 9, 2024

glyph commented Jun 9, 2024

adiroiban commented Jun 10, 2024

adiroiban Jun 10, 2024

adiroiban left a comment

glyph commented Jun 10, 2024

glyph commented Jun 10, 2024

glyph commented Jun 10, 2024

adiroiban commented Jun 20, 2024

glyph commented Jun 20, 2024

glyph commented Jun 20, 2024

glyph commented Jun 20, 2024

glyph commented Jun 20, 2024

adiroiban commented Jun 21, 2024

glyph commented Jun 21, 2024

adiroiban commented Jun 21, 2024


		:py:class:`Logger <twisted.logger.Logger>` provides a :py:meth:`failure <twisted.logger.Logger.failure>` method, which allows one to capture a :py:class:`Failure <twisted.python.failure.Failure>` object conveniently:
		:py:class:`Logger <twisted.logger.Logger>` provides a :py:meth:`failure <twisted.logger.Logger.handlingFailures>` method, which allows one to run some application code, then capture a :py:class:`Failure <twisted.python.failure.Failure>` in the log stream if that code raises an exception:

12188 add logging handler context managers #12207

12188 add logging handler context managers #12207

Conversation

glyph commented Jun 9, 2024

Scope and purpose

codspeed-hq bot commented Jun 9, 2024 • edited Loading

CodSpeed Performance Report

Merging #12207 will not alter performance

Summary

glyph commented Jun 9, 2024

glyph commented Jun 9, 2024

adiroiban commented Jun 10, 2024

adiroiban Jun 10, 2024

Choose a reason for hiding this comment

adiroiban left a comment

Choose a reason for hiding this comment

glyph commented Jun 10, 2024

glyph commented Jun 10, 2024

glyph commented Jun 10, 2024

adiroiban commented Jun 20, 2024

glyph commented Jun 20, 2024

glyph commented Jun 20, 2024

glyph commented Jun 20, 2024

glyph commented Jun 20, 2024

adiroiban commented Jun 21, 2024

glyph commented Jun 21, 2024

adiroiban commented Jun 21, 2024

codspeed-hq bot commented Jun 9, 2024 •

edited

Loading