Summary

Fix up arguments to logging methods.

Review Request #12704 — Created Oct. 28, 2022 and submitted Nov. 22, 2022, 3:15 p.m.

Information

Owner

david

Repository

Djblets

Branch

release-4.x

Bugs

Depends On

Reviewers

Groups

djblets

People

Description

We have a special logging formatter that is used to show info when a
django HttpRequest is part of the log record. Once upon a time, the way
of getting extra information into a log record was just to pass it as
additional kwargs to the logging methods, but the modern, documented
way of doing it is to pass a single extra dict. Some of our code uses
extra, but most of it is just passing in request= as a kwarg. I've
fixed this up, plus added the request in logging methods where we had it
available but weren't plumbing it.

In addition, there were a couple instances where we were passing
exc_info=1 instead of exc_info=True. I've fixed these up to make
type checkers happier.

Testing Done

Ran unit tests.

Commits

Summary	ID
Fix up arguments to logging methods. We have a special logging formatter that is used to show info when a django HttpRequest is part of the log record. Once upon a time, the way of getting extra information into a log record was just to pass it as additional `kwargs` to the logging methods, but the modern, documented way of doing it is to pass a single `extra` dict. Some of our code uses `extra`, but most of it is just passing in `request=` as a kwarg. I've fixed this up, plus added the request in logging methods where we had it available but weren't plumbing it. In addition, there were a couple instances where we were passing `exc_info=1` instead of `exc_info=True`. I've fixed these up to make type checkers happier. Testing Done: Ran unit tests.	afed7adb54a4fc231365c72880c7a0a3b8db783b

Summary

Fix up arguments to logging methods.

We have a special logging formatter that is used to show info when a django HttpRequest is part of the log record. Once upon a time, the way of getting extra information into a log record was just to pass it as additional `kwargs` to the logging methods, but the modern, documented way of doing it is to pass a single `extra` dict. Some of our code uses `extra`, but most of it is just passing in `request=` as a kwarg. I've fixed this up, plus added the request in logging methods where we had it available but weren't plumbing it. In addition, there were a couple instances where we were passing `exc_info=1` instead of `exc_info=True`. I've fixed these up to make type checkers happier. Testing Done: Ran unit tests.

afed7adb54a4fc231365c72880c7a0a3b8db783b

Issues

Description	From	Last Updated
The request= is actually a custom feature of our logging add-ons. extra= was historically the way custom data went in, …	chipx86	Nov. 22, 2022, 8:16 a.m.
Given the escaping for request, we should either use double quotes for the string itself or for the code sample.	chipx86	Nov. 22, 2022, 3:14 p.m.

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

The issue has been resolved. Show all issues

The request= is actually a custom feature of our logging add-ons. extra= was historically the way custom data went in, and is what request= ultimately does. So request= is actually API provided by djblets.log.

I still prefer request= just for the convenience of it, to be honest.

If we're for sure going to get rid of it, we should formally deprecate it.

david Nov. 17, 2022, 8:31 a.m.

I think I'd prefer to get rid of it. It's convenient, but type checkers and static analysis get mad, and monkey-patching standard lib stuff like this seems questionable at best.

Change Summary:

Add a deprecation warning

Commits:

	Summary	ID
	Fix up arguments to logging methods. We have a special logging formatter that is used to show info when a django HttpRequest is part of the log record. Once upon a time, the way of getting extra information into a log record was just to pass it as additional `kwargs` to the logging methods, but the modern, documented way of doing it is to pass a single `extra` dict. Some of our code uses `extra`, but most of it is just passing in `request=` as a kwarg. I've fixed this up, plus added the request in logging methods where we had it available but weren't plumbing it. In addition, there were a couple instances where we were passing `exc_info=1` instead of `exc_info=True`. I've fixed these up to make type checkers happier. Testing Done: Ran unit tests.	1c063cce8c0b820b1509e927d72b473d5894e990
	Fix up arguments to logging methods. We have a special logging formatter that is used to show info when a django HttpRequest is part of the log record. Once upon a time, the way of getting extra information into a log record was just to pass it as additional `kwargs` to the logging methods, but the modern, documented way of doing it is to pass a single `extra` dict. Some of our code uses `extra`, but most of it is just passing in `request=` as a kwarg. I've fixed this up, plus added the request in logging methods where we had it available but weren't plumbing it. In addition, there were a couple instances where we were passing `exc_info=1` instead of `exc_info=True`. I've fixed these up to make type checkers happier. Testing Done: Ran unit tests.	afed7adb54a4fc231365c72880c7a0a3b8db783b

Diff:

Revision 2 (+86 -58)

Show changes

	djblets/auth/ratelimit.py
	djblets/configforms/views.py
	djblets/datagrid/grids.py
	djblets/extensions/templatetags/djblets_extensions.py
	djblets/extensions/tests/test_extension_manager.py
	djblets/log/__init__.py
	djblets/log/middleware.py
	djblets/siteconfig/context_processors.py
	3 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Ship it!

djblets/log/__init__.py (Diff revisions 1 - 2)

The issue has been resolved. Show all issues

Given the escaping for request, we should either use double quotes for the string itself or for the code sample.

Status:: Completed
Change Summary:: Pushed to release-4.x (9f19dbb)