Add support to suppress tls logging #157
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
agbpatro
commented
Apr 11, 2024
| @@ -87,13 +121,22 @@ func (l *Logger) Log(logType LogType, message string, info LogInfo) { | |||
| } | |||
| } | |||
|
|
|||
| // Write implements the Write method of the io.Writer interface | |||
| func (l *Logger) Write(p []byte) (n int, err error) { | |||
There was a problem hiding this comment.
vmallet
reviewed
Apr 11, 2024
README.md
Outdated
| @@ -165,6 +165,10 @@ Configure and use Prometheus or a StatsD-interface supporting data store for met | |||
|
|
|||
|  | |||
|
|
|||
| ## Logging | |||
|
|
|||
| To supress [tls handshake error logging](https://cs.opensource.google/go/go/+/master:src/net/http/server.go;l=1933?q=%22TLS%20handshake%20error%20from%20%22&ss=go%2Fgo), set environment variable `SUPPRESSE_TLS_HANDSHAKE_ERROR_LOGGING` to true. See [docker compose](./docker-compose.yml) for example. | |||
There was a problem hiding this comment.
- To
suppress(typo)... SUPPRESS_TLS_HANDSHAKE_ERROR_LOGGING(typo, extraEinSUPPRESS)- Add backticks around
truemaybe?
logger/logger.go
Outdated
| @@ -34,29 +40,45 @@ type LogInfo map[string]interface{} | |||
| // Logger a logrus implementer of the JSON logger interface | |||
| type Logger struct { | |||
| logger *logrus.Entry | |||
|
|
|||
| supressionMessages []string | |||
There was a problem hiding this comment.
- typo in
suppression - How about:
suppressionFiltersmaybe?
logger/logger.go
Outdated
| return l | ||
| value, ok := os.LookupEnv("SUPPRESSE_TLS_HANDSHAKE_ERROR_LOGGING") | ||
| if ok { | ||
| ok, err := strconv.ParseBool(value) |
There was a problem hiding this comment.
How about b instead of ok here for the parsed boolean value? ok carries a slightly different meaning usually.
logger/logger.go
Outdated
| return nil, err | ||
| } | ||
| if ok { | ||
| suppressionMessages = append(suppressionMessages, tlsFilter) |
There was a problem hiding this comment.
That looks a bit convoluted for something that right now is going to stay with only one filter. Even though the suppressionFilters in the type is a slice, maybe when you set it up for now you just hardcoded to a slice of just that one filter, and we extend the code the day we actually need it? (just a thought).
logger/logger.go
Outdated
| @@ -69,8 +91,20 @@ func SetLogLevel(name string) { | |||
| logrus.SetLevel(level) | |||
| } | |||
|
|
|||
| func (l *Logger) shouldSuppress(message string) bool { | |||
| for _, suppressString := range l.supressionMessages { | |||
There was a problem hiding this comment.
suppressString -> filter, or f
logger/logger_logrus_test.go
Outdated
| Expect(len(hook.Entries)).To(Equal(4)) | ||
| Expect(hook.LastEntry().Level).To(Equal(githubLogrus.ErrorLevel)) | ||
| Expect(hook.LastEntry().Message).To(Equal("sample_anomaly_log_2")) | ||
|
|
||
| logger.Log(ERROR, "http: TLS handshake error from 0.0.0.0:1: EOF", nil) | ||
| Expect(len(hook.Entries)).To(Equal(5)) |
There was a problem hiding this comment.
Expect(hook.Entries).To(HaveLen(5))
logger/logger_logrus_test.go
Outdated
| }) | ||
|
|
||
| Context("Tls handshake error", func() { | ||
| DescribeTable("supresses", |
logger/logger_logrus_test.go
Outdated
| logger.Log(ERROR, "http: TLS handshake error from 0.0.0.0:1: EOF", nil) | ||
| Expect(hook.Entries).To(BeEmpty()) | ||
| logger.Log(ERROR, "Random error", nil) | ||
| Expect(len(hook.Entries)).To(Equal(1)) |
| Entry("for env variable 1", "1"), | ||
| ) | ||
| }) | ||
|
|
There was a problem hiding this comment.
Maybe add a case where the env var is set to true and we see it doesn't suppress other messages?
You're already doing this, sorry.
agbpatro
force-pushed
the
suppress_tls_handshake_errors
branch
5 times, most recently
from
31b556d
to
49349cc
Compare
vmallet
approved these changes
Apr 11, 2024
logger/logger.go
Outdated
| @@ -69,8 +89,18 @@ func SetLogLevel(name string) { | |||
| logrus.SetLevel(level) | |||
| } | |||
|
|
|||
| func (l *Logger) shouldSuppress(message string) bool { | |||
| if len(l.suppressionFilter) == 0 { | |||
There was a problem hiding this comment.
nit: usually go does empty string checks against "" instead of len, but that's fine too.
- Allow a way to suppress the logging messages related to tls handshake error logged in https://cs.opensource.google/go/go/+/master:src/net/http/server.go;l=1933?q=%22TLS%20handshake%20error%20from%20%22&ss=go%2Fgo. - Remove unused anomaly logger method
agbpatro
force-pushed
the
suppress_tls_handshake_errors
branch
from
49349cc
to
2beea99
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Please include a summary of the changes and the related issue.
Type of change
Key changes:-
Please select all options that apply to this change:
Checklist:
Confirm you have completed the following steps: