-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add silence-ping-logging flag #178
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.
Added a few comments inline, generally looks good but what do you think to making this ExcludePath
-> ExcludedPaths
, making it a list rather than a single item?
Might even be worth converting the list to a map[string]struct{}
inside the logger once initialised to make looking up excluded paths easier/quicker 🤔
I like this approach btw, it's something I'm trying to move towards with our other interfaces too |
Did some tests in cluster with Generate traffic (note I'm enforcing ro file system hence write error):
Pod logs:
|
Happy to amend, do you have an example interface in mind? |
@kskewes Could you fix up the merge conflicts and we can try get this merged this week? Thanks |
Add ability to silence logging of requests to /ping endpoint, reducing log clutter Pros: - Don't have to change all handlers to set/not set silent ping logging - Don't have to duplicate `loggingHandler` (this could be preferable yet) Cons: - Leaking oauth2proxy logic into `package logger` - Defining default pingPath in two locations Alternative: - Add generic exclude path to `logger.go` and pass in `/ping`.
Useful for excluding /ping endpoint to reduce log volume. This is somewhat more verbose than a simple bool to disable logging of the `/ping` endpoint. Perhaps better to add `-silence-ping-logging` bool flag to `options.go` and pass in the `/ping` endpoint as part of `logger` declaration in `options.go`. Could be extended into a slice of paths similar to go-gin's `SkipPaths`: https://github.com/gin-gonic/gin/blob/master/logger.go#L46
Can probably slim down the `ExcludePath` tests.
- Add `ping-path` option to enable switching on and passing to `logger.go` Default remains unchanged at: `"/ping"` - Add note in configuration.md about silence flag taking precedence Potential tests: - `options.go` sets `logger.SetExcludePath` based on silence flag? - Changing `PingPath` reflected in router?
- `logger.go` convert slice of paths to map for quicker lookup - `options.go` combines csv paths and pingpath into slice
Please hold whilst I verify the rebase in our clusters. |
Working for us subject to resolving this: #212 |
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 little thing and then we can merge
Co-Authored-By: Joel Speed <Joel.speed@hotmail.co.uk>
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.
Looking good! Thanks @kskewes
Thanks for patience and picking that up! |
Signed-off-by: bitliu <bitliu@tencent.com>
Avoid unnecessary pod restart on each helm chart version
Description
/ping
to a newOptions
PingPath
string so its value can be passed to other functions.Default remains unchanged at:
"/ping"
Options
ExcludeLoggingPaths
string to contain csv list ofurl.Path
's that should not be logged.Default
nil
= don't exclude any paths from logging.Options
SilencePingLogging
bool that addsPingPath
to theExcludeLoggingPaths
list, in effect silencing logging of requests to/ping
.Default
false
= don't silence logging of requests to/ping
.An alternative implementation could be to extend the
loggingHandler struct
withOptions
?Not sure, open to feedback and happy to revise as preferred.
Motivation and Context
Resolve issue: #167
Kubernetes health checking
/ping
endpoint creates unnecessary (for us) log entries.How Has This Been Tested?
Kubernetes 1.13 and 1.14 with Google Groups OAUTH.
Checklist: