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

Add query logging callbacks and context manager #1043

Merged
merged 12 commits into from Oct 9, 2023

Conversation

dcwatson
Copy link
Contributor

Another take on query logging (#629), with a similar approach to existing listeners. Consider this a WIP - I'm happy to write documentation and more tests if this approach is preferable to #630.

record = LoggedQuery(query, args, timeout, elapsed)
for cb in self._query_loggers:
if cb.is_async:
self._loop.create_task(cb.cb(con_ref, record))
Copy link
Member

Choose a reason for hiding this comment

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

Passing a con_ref is probably unnecessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I likely wouldn't use it, so happy to remove it, but I put it there so you could potentially log queries by host.

Copy link
Member

Choose a reason for hiding this comment

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

A concern here is potentially retaining references to free-d connections. Other callbacks take it, of course, but that's an API decision I've come to regret. Perhaps we can pass connection's addr and params instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works for me!

@@ -314,7 +339,11 @@ async def execute(self, query: str, *args, timeout: float=None) -> str:
self._check_open()

if not args:
return await self._protocol.query(query, timeout)
start = time.monotonic()
Copy link
Member

Choose a reason for hiding this comment

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

I think a context manager would probably be nicer/reduce duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, change incoming.

Comment on lines 1751 to 1759
with utils.timer() as t:
result, stmt = await self._do_execute(
query,
executor,
timeout,
record_class=record_class,
ignore_custom_codec=ignore_custom_codec,
)
self._log_query(query, args, timeout, t.elapsed)
Copy link
Member

Choose a reason for hiding this comment

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

You could go further and pull self._log_query into the context manager and that way you'll be able to log query errors too, e.g.

Suggested change
with utils.timer() as t:
result, stmt = await self._do_execute(
query,
executor,
timeout,
record_class=record_class,
ignore_custom_codec=ignore_custom_codec,
)
self._log_query(query, args, timeout, t.elapsed)
with self._time_and_log(query, args, timeout):
result, stmt = await self._do_execute(
query,
executor,
timeout,
record_class=record_class,
ignore_custom_codec=ignore_custom_codec,
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea, I feel silly for not considering error logging as part of this.

@@ -2327,6 +2414,12 @@ class _ConnectionProxy:
__slots__ = ()


LoggedQuery = collections.namedtuple(
'LoggedQuery',
['query', 'args', 'timeout', 'elapsed', 'exception', 'addr', 'params'])
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
['query', 'args', 'timeout', 'elapsed', 'exception', 'addr', 'params'])
['query', 'args', 'timeout', 'elapsed', 'exception', 'conn_addr', 'conn_params'])

@elprans elprans merged commit b2697ff into MagicStack:master Oct 9, 2023
33 checks passed
This was referenced Oct 9, 2023
elprans added a commit that referenced this pull request Nov 5, 2023
Minor fixes and improvements.

Improvements
============

* Python 3.12 and PostgreSQL 16 support (#1084)
  (by @elprans in deea86c)

* Add support for tuple-format custom codecs on composite types (#1061)
  (by @elprans in 922fcd1)

* Support `target_session_attrs` in URL format, add tests (#1073)
  (by @elprans in 7cb4e70)

* Infinity numeric support (#1067)
  (by @krokoziabla in 0c3bf60 for #1020)

* Add support for the `WHERE` clause in `copy_to` methods (#941)
  (by @kaylynn234 in b7ffab6)

* Add query logging callbacks and context manager (#1043)
  (by @dcwatson in b2697ff)

Fixes
=====

* When prepared statements are disabled, avoid relying on them harder (#1065)
  (by @elprans in cbf64e1)

* Handle environments with HOME set to a not-a-directory (#1063)
  (by @elprans in af922bc)

* Fix handling of non-ASCII passwords (#1062)
  (by @elprans in 89d5bd0)

* Disable JIT while doing type introspection (#1082)
  (by @elprans in f21ebf6)

* Remove connection parameter caching in `Pool` (#1053)
  (by @ermakov-oleg in 4ddb039)

* Switch to Python 3.12-style `wait_for` (#1086)
  (by @elprans in 4bdd8a7)

* Update automatic PostGIS type conversion for Shapely 2.0 (#1085)
  (by @ChimneySwift in 8b45beb)

* Use the `timeout` context manager in the connection path (#1087)
  (by @elprans in 313b2b2)

* Small fix for documentation on using SSL in Connection (#995)
  (by @ScottFred in ccc7baf)

* Use cleanup_ctx in pool usage doc (#878)
  (by @ir4y in 70c8bd8)

* Close cursor portals once the iterator is exhausted (#1088)
  (by @elprans in ca9f03b)

* Cut BaseProtocol circular reference on close. (#1049)
  (by @pteromys in 93a6f79)

* Allow passing hosts as tuples to `connect()` (in addition to lists) (#1021)
  (by @lezram in d7faaff)

Other
=====

* Drop support for Python 3.7 (#1064)
  (by @bryanforbes in 87ab143)
@elprans elprans mentioned this pull request Nov 5, 2023
elprans added a commit that referenced this pull request Nov 5, 2023
Minor fixes and improvements.

Improvements
============

* Python 3.12 and PostgreSQL 16 support (#1084)
  (by @elprans in deea86c)

* Add support for tuple-format custom codecs on composite types (#1061)
  (by @elprans in 922fcd1)

* Support `target_session_attrs` in URL format, add tests (#1073)
  (by @elprans in 7cb4e70)

* Infinity numeric support (#1067)
  (by @krokoziabla in 0c3bf60 for #1020)

* Add support for the `WHERE` clause in `copy_to` methods (#941)
  (by @kaylynn234 in b7ffab6)

* Add query logging callbacks and context manager (#1043)
  (by @dcwatson in b2697ff)

Fixes
=====

* When prepared statements are disabled, avoid relying on them harder (#1065)
  (by @elprans in cbf64e1)

* Handle environments with HOME set to a not-a-directory (#1063)
  (by @elprans in af922bc)

* Fix handling of non-ASCII passwords (#1062)
  (by @elprans in 89d5bd0)

* Disable JIT while doing type introspection (#1082)
  (by @elprans in f21ebf6)

* Remove connection parameter caching in `Pool` (#1053)
  (by @ermakov-oleg in 4ddb039)

* Switch to Python 3.12-style `wait_for` (#1086)
  (by @elprans in 4bdd8a7)

* Update automatic PostGIS type conversion for Shapely 2.0 (#1085)
  (by @ChimneySwift in 8b45beb)

* Use the `timeout` context manager in the connection path (#1087)
  (by @elprans in 313b2b2)

* Small fix for documentation on using SSL in Connection (#995)
  (by @ScottFred in ccc7baf)

* Use cleanup_ctx in pool usage doc (#878)
  (by @ir4y in 70c8bd8)

* Close cursor portals once the iterator is exhausted (#1088)
  (by @elprans in ca9f03b)

* Cut BaseProtocol circular reference on close. (#1049)
  (by @pteromys in 93a6f79)

* Allow passing hosts as tuples to `connect()` (in addition to lists) (#1021)
  (by @lezram in d7faaff)

Other
=====

* Drop support for Python 3.7 (#1064)
  (by @bryanforbes in 87ab143)
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.

None yet

2 participants