-
-
Notifications
You must be signed in to change notification settings - Fork 53
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
Allow for LogRequestMiddleware to filter the headers it outputs #433
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea but could be better implemented IMO.
I think we should aim for something like what traefik does.
It accepts a defaultMode
, and you can then provide overrides for specific headers. The setting options are keep
, redact
, drop
.
Given the default mode is none. Not sure I see the point of |
@adam-fowler it's useful if you want to log all headers, but not include a few. I've had usages for this in the past, personally. Generally, how traefik does it is the most flexible way, so users can just do whatever they want, without us making unnecessary assumptions about their needs. |
So you have two situations I want these specific headers or I want all headers except these ones. Adding an ignore list to |
Yes, an ignore list would work. But then I'd want to only redact some headers, not ignore them, so I'll need a redact list too |
I'm just looking at the redact list now. |
Example: show me all headers, redact Authorization or other sensitive headers, and ignore all access control headers. |
let accessControlHeaders: [HTTPField.Name] = [
.accessControlAllowCredentials,
.accessControlAllowHeaders,
.accessControlAllowMethods,
...
]
LogRequestsMiddleware(
.info,
includeHeaders: .all(except: accessControlHeaders),
redactHeaders: [.authorization]
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One concern, otherwise looks good (and thank you) 🙂
metadata: [ | ||
"hb_uri": .stringConvertible(request.uri), | ||
"hb_method": .string(request.method.rawValue), | ||
"hb_headers": .string(self.filterHeaders(headers: request.headers, filter: filter)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of formatting the headers instead of passing an e.g. Array or Dictionary as .stringConvertible
.
We use https://github.com/Brainfinance/StackdriverLogging as our swift-log backend which formats stuff properly as a JSON if it can, which makes them look decent on Google Cloud. By passing a String, a logging backend can't try to format the values anymore.
To be clear i haven't seen too many logging backends try to manually format metadata (my own DiscordLogger
doesn't either), but it does look useful when done appropriately, like with https://github.com/Brainfinance/StackdriverLogging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok makes sense. I can output a dictionary with values from duplicate keys concatenated with a "," inbetween
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Co-authored-by: Joannis Orlandos <[email protected]>
Co-authored-by: Joannis Orlandos <[email protected]>
Instead of displaying all the headers, only display ones you care about
Options are
.none
,.all
or.some([HTTPField.Name])