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

[#1784] fix runtime configuration for django-log-outgoing-requests #75

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

SonnyBA
Copy link
Contributor

@SonnyBA SonnyBA commented Sep 24, 2024

Partially fixes open-zaak/open-zaak#1784

Previously, setting the LOG_REQUESTS setting to False caused the runtime configuration of django-log-outgoing-requests to break. This was caused by no handlers being configured for its logger.

As the runtime configuration at the moment only allows configuring saving to the database (or not):

image

the save_outgoing_requests handler should always be included in the corresponding LOGGING setting.

Changes

Fixes the option to configure django-log-outgoing through the django admin.

@SonnyBA SonnyBA self-assigned this Sep 24, 2024
@SonnyBA
Copy link
Contributor Author

SonnyBA commented Sep 24, 2024

I do have to note that with all options available for django-log-outgoing-requests, it might be an idea to remove the LOG_REQUESTS option all together as the current configuration can be quite misleading (e.g allow django-log-outgoing-requests configuration to be done directly in an application).

Copy link
Contributor

@annashamray annashamray left a comment

Choose a reason for hiding this comment

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

I don't understand this change.
We use LOG_REQUESTS to turn off django-log-outgoing-requests completely in the same fashion as LOG_QUERIES for example

@SonnyBA
Copy link
Contributor Author

SonnyBA commented Sep 24, 2024

LOG_REQUESTS only enables (or disables) the configured LOGGING at the moment (see link in description). It is misleading that the user can configure django-log-outgoing-requests through the admin while it is completely disabled through configuration (the admin won't have any effect with the current setup). Currently whenever LOG_REQUESTS is set to False (the default), the admin interface is useless (as the LOGGING setting won't change).

@annashamray
Copy link
Contributor

Well this sounds like the configuration mistake on the client side. Probably they missed when we changed the default for LOG_REQUESTS to False (OZ 1.10.1)
I think saying that they need to explicitly provide this env var should be enough.
We had the same issue with Amsterdam (issue 15 in taiga)
Maybe we need to put a big warning in the CHANGELOG in OZ 1.10.1 for this change

About removing the env var in favor of the admin configuration:
Afaik the general trend that we support for APIs (which is inline with what clients want) is to move from admin configuration to configuration via env vars, so devops won't need to manually do anything, so if we remove some configuration to make it clearer it's better to remove the admin one.

Previously, setting the `LOG_REQUESTS` setting to `False` caused the
runtime configuration of django-log-outgoing-requests to break. This was
caused by no handlers being configured for its logger.

As the runtime configuration at the moment only allows configuring
saving to the database (or not) the `save_outgoing_requests` should
always be configured in the `LOGGING` setting.
@SonnyBA SonnyBA force-pushed the issue/1784-log-requests-setting branch from b00b29f to c2f3070 Compare October 1, 2024 10:27
Comment on lines +1049 to +1057
SOLO_CACHE = config(
"SOLO_CACHE",
default="default",
help_text=(
"The cache which will be used by Django Solo. Can be set to an empty "
"string to disable caching for Django Solo."
),
cast=lambda value: value if value else None,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

@annashamray do we want this to be configurable? I don't see a use case for clients to want to disable this/use a different cache

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with you, having the env var looks a little excessive, but I'm not strongly opposed to it. So it's up to you guys

Copy link
Contributor

Choose a reason for hiding this comment

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

@SonnyBA let's not make it an envvar and set it to "default" then

@annashamray
Copy link
Contributor

@SonnyBA I'm not approving this PR, so if my previous review blocks you feel free to dismiss it

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.

Outgoing request logging doesn't seem to work
3 participants