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

PB-1351 Log requests and errors to ECS #68

Merged
merged 7 commits into from
Jan 28, 2025

Conversation

schtibe
Copy link
Contributor

@schtibe schtibe commented Jan 23, 2025

Install ecs logging package and start sending log entries from the requests to our elastic stack

@schtibe schtibe force-pushed the feat-pb-1351-logging-facility branch from e4f308d to 85a7e05 Compare January 24, 2025 10:27
@schtibe schtibe requested a review from msom January 24, 2025 10:37
@schtibe schtibe force-pushed the feat-pb-1351-logging-facility branch 6 times, most recently from 5b0f45c to 708ea45 Compare January 24, 2025 16:44
Copy link
Contributor

@msom msom left a comment

Choose a reason for hiding this comment

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

LGTM but not sure if we need to already think about preventing logging credentials (for example the values of Authorization headers?

app/config/api.py Outdated Show resolved Hide resolved
app/config/api.py Outdated Show resolved Hide resolved
@schtibe schtibe requested a review from msom January 27, 2025 09:55
@schtibe schtibe force-pushed the feat-pb-1351-logging-facility branch 3 times, most recently from 4eea02c to 1e4227f Compare January 27, 2025 16:57
Send all API requests as well as the errors to the logging facility
It's already done in settings base
There was a distinction between 200s and 300s, but it resulted in both
logging on level `info`, thus squashing these log calls
@schtibe schtibe force-pushed the feat-pb-1351-logging-facility branch from 1e4227f to 8c0d075 Compare January 27, 2025 17:02
@msom
Copy link
Contributor

msom commented Jan 27, 2025

Also not sure about other Header Content, e.g. Set Cookie (https://security.stackexchange.com/questions/249442/storing-session-id-in-application-logs)

@msom
Copy link
Contributor

msom commented Jan 27, 2025

Can we define which headers we want to log? It just feels unsafe to generally write all headers to the log

@schtibe schtibe force-pushed the feat-pb-1351-logging-facility branch 2 times, most recently from 5932f25 to cae7d76 Compare January 28, 2025 10:59
@schtibe schtibe force-pushed the feat-pb-1351-logging-facility branch 3 times, most recently from 2326698 to 85a3a25 Compare January 28, 2025 15:00
Provide a white list that can be overwritten by the environment for which
headers we allow to go to the log
@schtibe schtibe force-pushed the feat-pb-1351-logging-facility branch from 85a3a25 to 1a1d76f Compare January 28, 2025 15:03
@schtibe schtibe merged commit b4f493b into develop Jan 28, 2025
3 checks passed
@schtibe schtibe deleted the feat-pb-1351-logging-facility branch January 28, 2025 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants