Skip to content

Conversation

@kingbuzzman
Copy link
Contributor

We have a lot of logs, including in celery workers. These workers as you can imagine won't ever have a request_id, in order to keep all our logs looking the same we normally put - as the missing value of our filters, nginx i think does the same, either way most of our filters default to that and yours is the only one that has none on it.

@kingbuzzman kingbuzzman changed the title Fixes #28 Adds the ability to change the default value of missing request_id Adds the ability to change the default value of missing request_id Apr 13, 2020
@kingbuzzman
Copy link
Contributor Author

On further inspection I see that issue #28 has nothing to do with this, as that relates to a missing middleware entry. Please let me know if there is anything else you want me to do, thanks.

request_id = local.request_id
except AttributeError:
request_id = NO_REQUEST_ID
request_id = DEFAULT_NO_REQUEST_ID
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this not be using the user-overidable value? Something like getattr(settings, LOG_REQUESTS_NO_SETTING, DEFAULT_NO_REQUEST_ID)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this is true, i dont use this part, but youre right. I'll change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok done, i should probably add a test, let me see where to put it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out there was no need for this, NO_REQUEST_ID is excluded from being sent anyways. I reworked it.

@RealOrangeOne RealOrangeOne merged commit 54d1afc into dabapps:master Apr 17, 2020
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.

2 participants