ansible / django-ansible-base

Apache License 2.0
11 stars 43 forks source link

Enhancements to request logging #478

Closed relrod closed 2 months ago

relrod commented 2 months ago

Signed-off-by: Rick Elrod rick@elrod.me

relrod commented 2 months ago

Since this has settings/uses settings, do we want to convert this into an app and have dynamic_settings set most required values?

I could go either way on this, but I have a slight inclination to not make this an app.

I think either way before this merges, I need to add docs for how to set it up.

I'd be interested in @AlanCoding's thoughts. I'm not sure what kind of overhead comes with having a lot of tiny, composable apps, vs just providing a library. I'm sure there's some overhead as Django has to look for migrations, signal handlers, etc, in each app initialization. But maybe it's very minimal.

Without an app, setting this up looks something like:

If we make it an app and do some magic mangling of LOGGING it becomes:

So the question is: Does any potential overhead that comes with an application outweigh the two extra steps that would otherwise be required?

AlanCoding commented 2 months ago

Since this has settings/uses settings, do we want to convert this into an app and have dynamic_settings set most required values?

Can I vote "no"?

I'm really not a fan of DAB making deep changes to data structures in settings. The base-level expectation is that it adds top-level variables to the settings when you do the include. If it has to modify something, may it's better to document these as expectations for the app using DAB. Sure, we already broke that rule in a few cases, but LOGGING is a massively complicated data structure which is seriously next-level.

We often modify LOGGING in multiple stages of settings. This can easily go wrong with unexpected KeyError type things, because of assumptions of what the partially-constructed data structure was at the start of your logic.

But what really kills :scream: is this:

Update the formatter(s) to have %(request_id)s somewhere

You're not just talking about modifying a dictionary 4-layers deep, but also string processing a format statement... and some formatters aren't even going to follow the rules you expect. What if you have a JSON formatter? That's going to need additional work on the app side, but if DAB is passing along an instruction to include request_id, then it has 100% finished its job, with no re-work on the DAB side needed. If you modify LOGGING, you will never stop adjusting.

relrod commented 2 months ago

but also string processing a format statement

Well no, I wouldn't do that from DAB, I'd leave it to the app to decide where in the format string to put the request id parameter. But yeah, I'm a bit squeamish about modifying LOGGING from DAB settings.

sonarcloud[bot] commented 2 months ago

Quality Gate Passed Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
96.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud