From feb53e786185b792a0b39b9a6d7d66d195390ca2 Mon Sep 17 00:00:00 2001 From: wiardvanrij Date: Sun, 22 Oct 2023 03:20:40 +0200 Subject: [PATCH 1/5] Improve reply handling and add a timeout in the http client (cherry picked from commit 5bfe5da8ed8288eeba43211abc4095fb9f3c9f27) --- go.sum | 9 +++++++ main.go | 5 +++- server.go | 76 +++++++++++++++++++++---------------------------------- 3 files changed, 42 insertions(+), 48 deletions(-) diff --git a/go.sum b/go.sum index d885b3f..544f62e 100644 --- a/go.sum +++ b/go.sum @@ -19,8 +19,10 @@ github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6r github.com/cespare/xxhash/v2 v2.2.0 h1:DC2CZ1Ep5Y4k3ZQ899DldepgrayRUGE6BBZ/cd9Cj44= github.com/cespare/xxhash/v2 v2.2.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/fsnotify/fsnotify v1.6.0 h1:n+5WquG0fcWoWp6xPWfHdbskMCQaFnG6PfBrh1Ky4HY= github.com/fsnotify/fsnotify v1.6.0/go.mod h1:sl3t1tCWJFWoRz9R8WJCbQihKKwmorjAbSClcnxKAGw= +github.com/go-logfmt/logfmt v0.5.1/go.mod h1:WYhtIu8zTZfxdn5+rREduYbwxfcBr/Vr6KEVveWlfTs= github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= github.com/golang/protobuf v1.5.0/go.mod h1:FsONVRAS9T7sI+LIUmWTfcYkHO4aIWwzhcaSAoJOfIk= github.com/golang/protobuf v1.5.3 h1:KhyjKVUg7Usr/dYsdSqoFveMYd5ko72D+zANwlG1mmg= @@ -29,8 +31,14 @@ github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/ github.com/google/go-cmp v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38= github.com/google/uuid v1.3.1 h1:KjJaJ9iWZ3jOFZIf1Lqf4laDRCasjl0BCmnEGxkdLb4= github.com/google/uuid v1.3.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= +github.com/jpillora/backoff v1.0.0/go.mod h1:J/6gKK9jxlEcS3zixgDgUAsiuZ7yrSoa/FX5e0EB2j4= +github.com/json-iterator/go v1.1.12/go.mod h1:e30LSqwooZae/UwlEbR2852Gd8hjQvJoHmT4TnhNGBo= +github.com/julienschmidt/httprouter v1.3.0/go.mod h1:JR6WtHb+2LUe8TCKY3cZOxFyyO8IZAc4RVcycCCAKdM= github.com/matttproud/golang_protobuf_extensions v1.0.4 h1:mmDVorXM7PCGKw94cs5zkfA9PSy5pEvNWRP0ET0TIVo= github.com/matttproud/golang_protobuf_extensions v1.0.4/go.mod h1:BSXmuO+STAnVfrANrmjBb36TMTDstsz7MSK+HVaYKv4= +github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= +github.com/modern-go/reflect2 v1.0.2/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjYzDa0/r8luk= +github.com/mwitkow/go-conntrack v0.0.0-20190716064945-2f068394615f/go.mod h1:qRWi+5nqEBWmkhHvq77mSJWrCKwh8bxhgT7d/eI7P4U= github.com/prometheus/client_golang v1.17.0 h1:rl2sfwZMtSthVU752MqfjQozy7blglC+1SOtjMAMh+Q= github.com/prometheus/client_golang v1.17.0/go.mod h1:VeL+gMmOAxkS2IqfCq0ZmHSL+LjWfWDUmp1mBz9JgUY= github.com/prometheus/client_model v0.4.1-0.20230718164431-9a2bf3000d16 h1:v7DLqVdK4VrYkVD5diGdl4sxJurKJEMnODWRJlxV9oM= @@ -56,3 +64,4 @@ google.golang.org/protobuf v1.26.0-rc.1/go.mod h1:jlhhOSvTdKEhbULTjvd4ARK9grFBp0 google.golang.org/protobuf v1.26.0/go.mod h1:9q0QmTI4eRPtz6boOQmLYwt+qCgq0jsYwAQnmE0givc= google.golang.org/protobuf v1.31.0 h1:g0LDEJHgrBl9N9r17Ru3sqWhkIx2NB67okBHPwC7hs8= google.golang.org/protobuf v1.31.0/go.mod h1:HV8QOd/L58Z+nl8r43ehVNZIU/HEI6OcFqwMG9pJV4I= +gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ= diff --git a/main.go b/main.go index 0660019..cbaccd9 100644 --- a/main.go +++ b/main.go @@ -7,6 +7,7 @@ import ( "flag" "net/http" "sync" + "time" "fortio.org/log" "fortio.org/scli" @@ -78,7 +79,9 @@ func main() { metrics := NewMetrics(r) // Initialize the app, metrics are passed along so they are accessible - app := NewApp(maxQueueSize, &http.Client{}, metrics) + app := NewApp(maxQueueSize, &http.Client{ + Timeout: 10 * time.Second, + }, metrics) // The only required flag is the token at the moment. if tokenFlag == "" { log.Fatalf("Missing token flag") diff --git a/server.go b/server.go index f4a9cc9..d3214b6 100644 --- a/server.go +++ b/server.go @@ -10,6 +10,7 @@ import ( "strings" "fortio.org/fortio/fhttp" + "fortio.org/fortio/jrpc" "fortio.org/log" ) @@ -45,14 +46,9 @@ func (app *App) StartServer(ctx context.Context, applicationPort string) error { } func (app *App) handleRequest(w http.ResponseWriter, r *http.Request) { - // Regardless of the outcome, we always respond as json - w.Header().Set("Content-Type", "application/json") - - // "Mock" the response from Slack. - // OK is true by default, so we only need to set it to false if we want to trow an error which then could use a custom error message. - // From testing, any application only checks if OK is true. So we can ignore all other fields - fakeSlackResponse := SlackResponse{ - Ok: true, + if r.Method != http.MethodPost { + http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) + return } maxQueueSize := int(float64(cap(app.slackQueue)) * 0.9) @@ -61,17 +57,13 @@ func (app *App) handleRequest(w http.ResponseWriter, r *http.Request) { // Ideally we don't reject at 90%, but initially after some tests I got blocked. So I decided to be a bit more conservative. // ToDo: Fix this behavior so we can reach 100% channel size without problems. if len(app.slackQueue) >= maxQueueSize { - w.WriteHeader(http.StatusServiceUnavailable) + log.S(log.Info, "Queue is almost full, returning StatusServiceUnavailable", log.Int("queueSize", len(app.slackQueue))) - fakeSlackResponse.Ok = false - fakeSlackResponse.Error = "Queue is almost full" - responseData, err := json.Marshal(fakeSlackResponse) - if err != nil { - http.Error(w, "Failed to serialize Slack response", http.StatusInternalServerError) - return - } + err := jrpc.Reply[SlackResponse](w, http.StatusServiceUnavailable, &SlackResponse{ + Ok: false, + Error: "Queue is almost full", + }) - _, err = w.Write(responseData) if err != nil { log.S(log.Error, "Failed to write response", log.Any("err", err)) } @@ -79,42 +71,30 @@ func (app *App) handleRequest(w http.ResponseWriter, r *http.Request) { return } - if r.Method != http.MethodPost { - http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) - return - } - var request SlackPostMessageRequest - err := json.NewDecoder(r.Body).Decode(&request) - if err != nil { - http.Error(w, "Failed to read the request body", http.StatusInternalServerError) - return - } + requestErr := json.NewDecoder(r.Body).Decode(&request) - // Validate the request - err = validate(request) - if err != nil { - log.S(log.Error, "Invalid request", log.Any("err", err)) - // TODO: jrpc.(Client)ErrorReply ? - w.WriteHeader(http.StatusBadRequest) - fakeSlackResponse.Ok = false - fakeSlackResponse.Error = err.Error() - responseData, err2 := json.Marshal(fakeSlackResponse) - _, err3 := w.Write(responseData) - if err2 != nil || err3 != nil { - log.S(log.Error, "Failed to write response", log.Any("err2", err2), log.Any("err3", err3)) - } - return + // If we can't decode, we don't bother validating. In the end it's the same outcome if either one is invalid. + if requestErr == nil { + requestErr = validate(request) } - app.metrics.RequestsReceivedTotal.WithLabelValues(request.Channel).Inc() + if requestErr != nil { + log.S(log.Error, "Invalid request", log.Any("err", requestErr)) - responseData, err := json.Marshal(fakeSlackResponse) - if err != nil { - http.Error(w, "Failed to serialize Slack response", http.StatusInternalServerError) + err := jrpc.Reply[SlackResponse](w, http.StatusBadRequest, &SlackResponse{ + Ok: false, + Error: requestErr.Error(), + }) + + if err != nil { + log.S(log.Error, "Failed to write response", log.Any("err", err)) + } return } + // Start the logic (as we passed all our checks) to process the request. + app.metrics.RequestsReceivedTotal.WithLabelValues(request.Channel).Inc() // Add a counter to the wait group, this is important to wait for all the messages to be processed before shutting down the server. app.wg.Add(1) // Send the message to the slackQueue to be processed @@ -126,8 +106,10 @@ func (app *App) handleRequest(w http.ResponseWriter, r *http.Request) { // This is the downside of having a queue which could potentially delay responses by a lot. // We do our due diligences on the received message and can make a fair assumption we will be able to process it. // Application should utlise this applications metrics and logs to find out if there are any issues. - w.WriteHeader(http.StatusOK) - _, err = w.Write(responseData) + err := jrpc.Reply[SlackResponse](w, http.StatusOK, &SlackResponse{ + Ok: true, + }) + if err != nil { log.S(log.Error, "Failed to write response", log.Any("err", err)) } From 0857a2ad66e62e6f03f8d2887c020630b99157bc Mon Sep 17 00:00:00 2001 From: Laurent Demailly Date: Sat, 21 Oct 2023 19:07:41 -0700 Subject: [PATCH 2/5] linter fixes --- server.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/server.go b/server.go index d3214b6..5b609fc 100644 --- a/server.go +++ b/server.go @@ -63,7 +63,6 @@ func (app *App) handleRequest(w http.ResponseWriter, r *http.Request) { Ok: false, Error: "Queue is almost full", }) - if err != nil { log.S(log.Error, "Failed to write response", log.Any("err", err)) } @@ -86,7 +85,6 @@ func (app *App) handleRequest(w http.ResponseWriter, r *http.Request) { Ok: false, Error: requestErr.Error(), }) - if err != nil { log.S(log.Error, "Failed to write response", log.Any("err", err)) } @@ -109,7 +107,6 @@ func (app *App) handleRequest(w http.ResponseWriter, r *http.Request) { err := jrpc.Reply[SlackResponse](w, http.StatusOK, &SlackResponse{ Ok: true, }) - if err != nil { log.S(log.Error, "Failed to write response", log.Any("err", err)) } From 16e67af520764dad26b18543734c0836b595d9a2 Mon Sep 17 00:00:00 2001 From: Laurent Demailly Date: Sat, 21 Oct 2023 19:10:19 -0700 Subject: [PATCH 3/5] missing go mod tidy --- go.sum | 9 --------- 1 file changed, 9 deletions(-) diff --git a/go.sum b/go.sum index 544f62e..d885b3f 100644 --- a/go.sum +++ b/go.sum @@ -19,10 +19,8 @@ github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6r github.com/cespare/xxhash/v2 v2.2.0 h1:DC2CZ1Ep5Y4k3ZQ899DldepgrayRUGE6BBZ/cd9Cj44= github.com/cespare/xxhash/v2 v2.2.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= -github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/fsnotify/fsnotify v1.6.0 h1:n+5WquG0fcWoWp6xPWfHdbskMCQaFnG6PfBrh1Ky4HY= github.com/fsnotify/fsnotify v1.6.0/go.mod h1:sl3t1tCWJFWoRz9R8WJCbQihKKwmorjAbSClcnxKAGw= -github.com/go-logfmt/logfmt v0.5.1/go.mod h1:WYhtIu8zTZfxdn5+rREduYbwxfcBr/Vr6KEVveWlfTs= github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= github.com/golang/protobuf v1.5.0/go.mod h1:FsONVRAS9T7sI+LIUmWTfcYkHO4aIWwzhcaSAoJOfIk= github.com/golang/protobuf v1.5.3 h1:KhyjKVUg7Usr/dYsdSqoFveMYd5ko72D+zANwlG1mmg= @@ -31,14 +29,8 @@ github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/ github.com/google/go-cmp v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38= github.com/google/uuid v1.3.1 h1:KjJaJ9iWZ3jOFZIf1Lqf4laDRCasjl0BCmnEGxkdLb4= github.com/google/uuid v1.3.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= -github.com/jpillora/backoff v1.0.0/go.mod h1:J/6gKK9jxlEcS3zixgDgUAsiuZ7yrSoa/FX5e0EB2j4= -github.com/json-iterator/go v1.1.12/go.mod h1:e30LSqwooZae/UwlEbR2852Gd8hjQvJoHmT4TnhNGBo= -github.com/julienschmidt/httprouter v1.3.0/go.mod h1:JR6WtHb+2LUe8TCKY3cZOxFyyO8IZAc4RVcycCCAKdM= github.com/matttproud/golang_protobuf_extensions v1.0.4 h1:mmDVorXM7PCGKw94cs5zkfA9PSy5pEvNWRP0ET0TIVo= github.com/matttproud/golang_protobuf_extensions v1.0.4/go.mod h1:BSXmuO+STAnVfrANrmjBb36TMTDstsz7MSK+HVaYKv4= -github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= -github.com/modern-go/reflect2 v1.0.2/go.mod h1:yWuevngMOJpCy52FWWMvUC8ws7m/LJsjYzDa0/r8luk= -github.com/mwitkow/go-conntrack v0.0.0-20190716064945-2f068394615f/go.mod h1:qRWi+5nqEBWmkhHvq77mSJWrCKwh8bxhgT7d/eI7P4U= github.com/prometheus/client_golang v1.17.0 h1:rl2sfwZMtSthVU752MqfjQozy7blglC+1SOtjMAMh+Q= github.com/prometheus/client_golang v1.17.0/go.mod h1:VeL+gMmOAxkS2IqfCq0ZmHSL+LjWfWDUmp1mBz9JgUY= github.com/prometheus/client_model v0.4.1-0.20230718164431-9a2bf3000d16 h1:v7DLqVdK4VrYkVD5diGdl4sxJurKJEMnODWRJlxV9oM= @@ -64,4 +56,3 @@ google.golang.org/protobuf v1.26.0-rc.1/go.mod h1:jlhhOSvTdKEhbULTjvd4ARK9grFBp0 google.golang.org/protobuf v1.26.0/go.mod h1:9q0QmTI4eRPtz6boOQmLYwt+qCgq0jsYwAQnmE0givc= google.golang.org/protobuf v1.31.0 h1:g0LDEJHgrBl9N9r17Ru3sqWhkIx2NB67okBHPwC7hs8= google.golang.org/protobuf v1.31.0/go.mod h1:HV8QOd/L58Z+nl8r43ehVNZIU/HEI6OcFqwMG9pJV4I= -gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ= From 69932373a9cb853d40d9ecc3678ebfc581ec98e0 Mon Sep 17 00:00:00 2001 From: Laurent Demailly Date: Sat, 21 Oct 2023 19:14:10 -0700 Subject: [PATCH 4/5] fix coverage setup --- .github/workflows/codecov.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/codecov.yml b/.github/workflows/codecov.yml index 5677bdc..1c05fca 100644 --- a/.github/workflows/codecov.yml +++ b/.github/workflows/codecov.yml @@ -2,10 +2,10 @@ name: Test Coverage on: push: - branches: [ master ] + branches: [ main ] pull_request: # The branches below must be a subset of the branches above - branches: [ master ] + branches: [ main ] jobs: build: From ed57868b8eef0390b90867b41c8048e39237cfc1 Mon Sep 17 00:00:00 2001 From: Laurent Demailly Date: Sat, 21 Oct 2023 19:19:59 -0700 Subject: [PATCH 5/5] take working coverage action from fortio/log --- .github/workflows/codecov.yml | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/.github/workflows/codecov.yml b/.github/workflows/codecov.yml index 1c05fca..b00ec1e 100644 --- a/.github/workflows/codecov.yml +++ b/.github/workflows/codecov.yml @@ -1,25 +1,21 @@ -name: Test Coverage +name: "Code Coverage" on: push: branches: [ main ] pull_request: - # The branches below must be a subset of the branches above branches: [ main ] jobs: - build: + coverage: runs-on: ubuntu-latest steps: - - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # pin@v3 - with: - fetch-depth: 2 - - name: Set up Go - uses: actions/setup-go@93397bea11091df50f3d7e59dc26a7711a8bcfbe # pin@v4 - with: + - uses: actions/checkout@master + - uses: actions/setup-go@v4 + with: go-version: '1.20' - check-latest: true - - name: Run coverage - run: make coverage - - name: Upload coverage to Codecov - uses: codecov/codecov-action@eaaf4bedf32dbdc6b720b63067d99c4d77d6047d # pin@v3 + - name: Run test coverage + run: go test -race -coverprofile=coverage.out -covermode=atomic ./... + - uses: codecov/codecov-action@v3 + with: + files: coverage.out