httpbp: Set http.Server.ErrorLog #576
Conversation
fishy
requested review from
konradreiche and
ghirsch-reddit
and removed request for
a team
| // Should not happen, but if it really happens, we just fallback to stdlib | ||
| // logger, which is not that big a deal either. |
There was a problem hiding this comment.
Is this because we treat a nil logger as the stdlib logger somewhere? Where does that happen?
There was a problem hiding this comment.
httpbp/error_logger.go
Outdated
| }, []string{"upstream_issue"}) | ||
|
|
||
| type issueConfig struct { | ||
| rate float64 |
There was a problem hiding this comment.
| rate float64 | |
| sampleRate float64 |
There's not much to example. Users can supply the map config on how much they want to suppress the logs. If they don't change any code beyond updating baseplate.go, we fallback to 100% sample, so they get the free counter and the log will be in proper json format (instead of using stdlib logger). |
| logger.Printf("Hello, golang.org/issue/25192!") | ||
| if got, want := sb.String(), `{"level":"warn","msg":"Hello, golang.org/issue/25192!","from":"http-server"}`; strings.TrimSpace(got) != want { | ||
| t.Errorf("Got %q, want %q", got, want) | ||
| } | ||
|
|
||
| sb.Reset() | ||
| logger.Printf("Hello, world!") | ||
| if got, want := sb.String(), `{"level":"warn","msg":"Hello, world!","from":"http-server"}`; strings.TrimSpace(got) != want { | ||
| t.Errorf("Got %q, want %q", got, want) | ||
| } |
There was a problem hiding this comment.
🔕 just a nit that you do this differently on line 63
There was a problem hiding this comment.
that's intentional. it's a long string so I actually prefer to have it in its own line as a const. but in this case I need it twice so a const no longer works.
httpbp/error_logger.go
Outdated
| // Example message: | ||
| // URL query contains semicolon, which is no longer a supported separator; parts of the query may be stripped when parsed; see golang.org/issue/25192 | ||
| if strings.Contains(entry.Message, "golang.org/issue/25192") { | ||
| cfg := w.issues[25192] |
There was a problem hiding this comment.
I don't think the map is really helping you here. Since the error message is hard-coded and we are hard coding the supported IDs and so on, I would make a field for each one and just do
w.issue25192.Inc()
etc.
| sb.Reset() | ||
| logger.Printf("Hello, world!") | ||
| const want = `{"level":"warn","msg":"Hello, world!","from":"http-server"}` | ||
| if got := sb.String(); strings.TrimSpace(got) != want { |
There was a problem hiding this comment.
🔕 cmp.Diff works well for strings like this, it calls out the part that changed instead of making a human squint at two almost identical lines
There was a problem hiding this comment.
the problem with cmp.Diff is that you can't declare and assign all of got, want, diff in the same if statement :)
There was a problem hiding this comment.
Yeah, that is definitely a bummer. I do
if diff := cmp.Diff(got, want); diff != "" {
t.Errorf("<context> (-got +want)\n%s", diff)
}
which 🤷
httpbp/server.go
Outdated
| // When a supported issue id is not in the map, | ||
| // currently the default sample rate is 1 | ||
| // (100% sampled, e.g. not dropping those logs). | ||
| // This might be changed to 0 in the future. |
There was a problem hiding this comment.
🔕 I'm in kindof a YAGNI mode here... this is a lot of complexity to carry around for something we might not need.
| sb.Reset() | ||
| logger.Printf("Hello, world!") | ||
| const want = `{"level":"warn","msg":"Hello, world!","from":"http-server"}` | ||
| if got := sb.String(); strings.TrimSpace(got) != want { |
There was a problem hiding this comment.
Yeah, that is definitely a bummer. I do
if diff := cmp.Diff(got, want); diff != "" {
t.Errorf("<context> (-got +want)\n%s", diff)
}
which 🤷
It's used by upstream server to log some of the kinda-breaking issues, for example [1]. Since the majority of the http servers at Reddit are public facing, when those happens it's usually just user messing with us, not really that the http client we control doing things wrong. This gives us a way to suppress those logs, and also emit counters to better track how many times those happened. This also makes the upstream http server to use the same json logger by zap as the rest of our code, at warning level. [1]: golang/go#25192 (comment)
It's used by upstream server to log some of the kinda-breaking issues,
for example 1. Since the majority of the http servers at Reddit are
public facing, when those happens it's usually just user messing with
us, not really that the http client we control doing things wrong. This
gives us a way to suppress those logs, and also emit counters to better
track how many times those happened.
This also makes the upstream http server to use the same json logger by
zap as the rest of our code, at warning level.
(I still need to write tests for it, but send it out first to get some early eyes/feedback)done