Merge branch 'better-errors' into 'master'

Improve errors

- Add request metadata (method, path)
- Sentry improvement: custom types for common errors

Closes https://gitlab.com/gitlab-org/gitlab-workhorse/issues/64

See merge request !69

internal/api/api.go

@@ -147,13 +147,13 @@ func (api *API) PreAuthorizeHandler(h HandleFunc, suffix string) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		authReq, err := api.newRequest(r, nil, suffix)
 		if err != nil {
-			helper.Fail500(w, fmt.Errorf("preAuthorizeHandler: newUpstreamRequest: %v", err))
+			helper.Fail500(w, r, fmt.Errorf("preAuthorizeHandler newUpstreamRequest: %v", err))
 			return
 		}
 
 		authResponse, err := api.Client.Do(authReq)
 		if err != nil {
-			helper.Fail500(w, fmt.Errorf("preAuthorizeHandler: do %v: %v", authReq.URL.Path, err))
+			helper.Fail500(w, r, fmt.Errorf("preAuthorizeHandler: do request: %v", err))
 			return
 		}
 		defer authResponse.Body.Close()
@@ -173,7 +173,7 @@ func (api *API) PreAuthorizeHandler(h HandleFunc, suffix string) http.Handler {
 		}
 
 		if contentType := authResponse.Header.Get("Content-Type"); contentType != ResponseContentType {
-			helper.Fail500(w, fmt.Errorf("preAuthorizeHandler: API responded with wrong content type: %v", contentType))
+			helper.Fail500(w, r, fmt.Errorf("preAuthorizeHandler: API responded with wrong content type: %v", contentType))
 			return
 		}
 
@@ -182,7 +182,7 @@ func (api *API) PreAuthorizeHandler(h HandleFunc, suffix string) http.Handler {
 		// request metadata. We must extract this information from the auth
 		// response body.
 		if err := json.NewDecoder(authResponse.Body).Decode(a); err != nil {
-			helper.Fail500(w, fmt.Errorf("preAuthorizeHandler: decode authorization response: %v", err))
+			helper.Fail500(w, r, fmt.Errorf("preAuthorizeHandler: decode authorization response: %v", err))
 			return
 		}
 		// Don't hog a TCP connection in CLOSE_WAIT, we can already close it now

internal/api/block.go

@@ -11,7 +11,7 @@ import (
 // leaking to the end user
 func Block(h http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		rw := &blocker{rw: w}
+		rw := &blocker{rw: w, r: r}
 		defer rw.Flush()
 		h.ServeHTTP(rw, r)
 	})
@@ -19,6 +19,7 @@ func Block(h http.Handler) http.Handler {
 
 type blocker struct {
 	rw       http.ResponseWriter
+	r        *http.Request
 	hijacked bool
 	status   int
 }
@@ -47,7 +48,7 @@ func (b *blocker) WriteHeader(status int) {
 		b.status = 500
 		b.Header().Del("Content-Length")
 		b.hijacked = true
-		helper.Fail500(b.rw, fmt.Errorf("api.blocker: forbidden content-type: %q", ResponseContentType))
+		helper.Fail500(b.rw, b.r, fmt.Errorf("api.blocker: forbidden content-type: %q", ResponseContentType))
 		return
 	}
 

internal/artifacts/artifacts_upload.go

 package artifacts
 
 import (
-	"errors"
 	"fmt"
 	"io/ioutil"
 	"mime/multipart"
@@ -75,7 +74,7 @@ func (a *artifactsUploadProcessor) Cleanup() {
 func UploadArtifacts(myAPI *api.API, h http.Handler) http.Handler {
 	return myAPI.PreAuthorizeHandler(func(w http.ResponseWriter, r *http.Request, a *api.Response) {
 		if a.TempPath == "" {
-			helper.Fail500(w, errors.New("UploadArtifacts: TempPath is empty"))
+			helper.Fail500(w, r, fmt.Errorf("UploadArtifacts: TempPath is empty"))
 			return
 		}
 

internal/artifacts/entry.go

@@ -2,7 +2,6 @@ package artifacts
 
 import (
 	"bufio"
-	"errors"
 	"fmt"
 	"io"
 	"log"
@@ -28,14 +27,14 @@ var SendEntry = &entry{"artifacts-entry:"}
 func (e *entry) Inject(w http.ResponseWriter, r *http.Request, sendData string) {
 	var params entryParams
 	if err := e.Unpack(&params, sendData); err != nil {
-		helper.Fail500(w, fmt.Errorf("SendEntry: unpack sendData: %v", err))
+		helper.Fail500(w, r, fmt.Errorf("SendEntry: unpack sendData: %v", err))
 		return
 	}
 
 	log.Printf("SendEntry: sending %q from %q for %q", params.Entry, params.Archive, r.URL.Path)
 
 	if params.Archive == "" || params.Entry == "" {
-		helper.Fail500(w, errors.New("SendEntry: Archive or Entry is empty"))
+		helper.Fail500(w, r, fmt.Errorf("SendEntry: Archive or Entry is empty"))
 		return
 	}
 
@@ -44,7 +43,7 @@ func (e *entry) Inject(w http.ResponseWriter, r *http.Request, sendData string)
 	if os.IsNotExist(err) {
 		http.NotFound(w, r)
 	} else if err != nil {
-		helper.Fail500(w, fmt.Errorf("SendEntry: %v", err))
+		helper.Fail500(w, r, fmt.Errorf("SendEntry: %v", err))
 	}
 }
 

internal/badgateway/roundtripper.go

@@ -25,7 +25,7 @@ var DefaultTransport = &http.Transport{
 }
 
 // Custom error for pretty Sentry 'issues'
-type Error502 error
+type Error struct{ error }
 
 type RoundTripper struct {
 	Transport *http.Transport
@@ -81,9 +81,10 @@ func (t *RoundTripper) RoundTrip(r *http.Request) (res *http.Response, err error
 	// instead of 500s we catch the RoundTrip error here and inject a
 	// 502 response.
 	if err != nil {
-		helper.LogError(Error502(fmt.Errorf("badgateway: %s %q failed after %.3fs: %v",
-			r.Method, r.RequestURI, time.Since(start).Seconds(), err,
-		)))
+		helper.LogError(
+			r,
+			&Error{fmt.Errorf("badgateway: failed after %.3fs: %v", time.Since(start).Seconds(), err)},
+		)
 
 		res = &http.Response{
 			StatusCode: http.StatusBadGateway,

internal/git/archive.go

@@ -34,7 +34,7 @@ var SendArchive = &archive{"git-archive:"}
 func (a *archive) Inject(w http.ResponseWriter, r *http.Request, sendData string) {
 	var params archiveParams
 	if err := a.Unpack(&params, sendData); err != nil {
-		helper.Fail500(w, fmt.Errorf("SendArchive: unpack sendData: %v", err))
+		helper.Fail500(w, r, fmt.Errorf("SendArchive: unpack sendData: %v", err))
 		return
 	}
 
@@ -42,7 +42,7 @@ func (a *archive) Inject(w http.ResponseWriter, r *http.Request, sendData string
 	urlPath := r.URL.Path
 	format, ok := parseBasename(filepath.Base(urlPath))
 	if !ok {
-		helper.Fail500(w, fmt.Errorf("handleGetArchive: invalid format: %s", urlPath))
+		helper.Fail500(w, r, fmt.Errorf("SendArchive: invalid format: %s", urlPath))
 		return
 	}
 
@@ -65,7 +65,7 @@ func (a *archive) Inject(w http.ResponseWriter, r *http.Request, sendData string
 	// to finalize the cached archive.
 	tempFile, err := prepareArchiveTempfile(path.Dir(params.ArchivePath), archiveFilename)
 	if err != nil {
-		helper.Fail500(w, fmt.Errorf("handleGetArchive: create tempfile: %v", err))
+		helper.Fail500(w, r, fmt.Errorf("SendArchive: create tempfile: %v", err))
 		return
 	}
 	defer tempFile.Close()
@@ -76,12 +76,12 @@ func (a *archive) Inject(w http.ResponseWriter, r *http.Request, sendData string
 	archiveCmd := gitCommand("", "git", "--git-dir="+params.RepoPath, "archive", "--format="+archiveFormat, "--prefix="+params.ArchivePrefix+"/", params.CommitId)
 	archiveStdout, err := archiveCmd.StdoutPipe()
 	if err != nil {
-		helper.Fail500(w, fmt.Errorf("handleGetArchive: archive stdout: %v", err))
+		helper.Fail500(w, r, fmt.Errorf("SendArchive: archive stdout: %v", err))
 		return
 	}
 	defer archiveStdout.Close()
 	if err := archiveCmd.Start(); err != nil {
-		helper.Fail500(w, fmt.Errorf("handleGetArchive: start %v: %v", archiveCmd.Args, err))
+		helper.Fail500(w, r, fmt.Errorf("SendArchive: start %v: %v", archiveCmd.Args, err))
 		return
 	}
 	defer helper.CleanUpProcessGroup(archiveCmd) // Ensure brute force subprocess clean-up
@@ -95,13 +95,13 @@ func (a *archive) Inject(w http.ResponseWriter, r *http.Request, sendData string
 
 		stdout, err = compressCmd.StdoutPipe()
 		if err != nil {
-			helper.Fail500(w, fmt.Errorf("handleGetArchive: compress stdout: %v", err))
+			helper.Fail500(w, r, fmt.Errorf("SendArchive: compress stdout: %v", err))
 			return
 		}
 		defer stdout.Close()
 
 		if err := compressCmd.Start(); err != nil {
-			helper.Fail500(w, fmt.Errorf("handleGetArchive: start %v: %v", compressCmd.Args, err))
+			helper.Fail500(w, r, fmt.Errorf("SendArchive: start %v: %v", compressCmd.Args, err))
 			return
 		}
 		defer helper.CleanUpProcessGroup(compressCmd)
@@ -116,22 +116,22 @@ func (a *archive) Inject(w http.ResponseWriter, r *http.Request, sendData string
 	setArchiveHeaders(w, format, archiveFilename)
 	w.WriteHeader(200) // Don't bother with HTTP 500 from this point on, just return
 	if _, err := io.Copy(w, archiveReader); err != nil {
-		helper.LogError(fmt.Errorf("handleGetArchive: copy 'git archive' output: %v", err))
+		helper.LogError(r, &copyError{fmt.Errorf("SendArchive: copy 'git archive' output: %v", err)})
 		return
 	}
 	if err := archiveCmd.Wait(); err != nil {
-		helper.LogError(fmt.Errorf("handleGetArchive: archiveCmd: %v", err))
+		helper.LogError(r, fmt.Errorf("SendArchive: archiveCmd: %v", err))
 		return
 	}
 	if compressCmd != nil {
 		if err := compressCmd.Wait(); err != nil {
-			helper.LogError(fmt.Errorf("handleGetArchive: compressCmd: %v", err))
+			helper.LogError(r, fmt.Errorf("SendArchive: compressCmd: %v", err))
 			return
 		}
 	}
 
 	if err := finalizeCachedArchive(tempFile, params.ArchivePath); err != nil {
-		helper.LogError(fmt.Errorf("handleGetArchive: finalize cached archive: %v", err))
+		helper.LogError(r, fmt.Errorf("SendArchive: finalize cached archive: %v", err))
 		return
 	}
 }

internal/git/blob.go

@@ -19,7 +19,7 @@ var SendBlob = &blob{"git-blob:"}
 func (b *blob) Inject(w http.ResponseWriter, r *http.Request, sendData string) {
 	var params blobParams
 	if err := b.Unpack(&params, sendData); err != nil {
-		helper.Fail500(w, fmt.Errorf("SendBlob: unpack sendData: %v", err))
+		helper.Fail500(w, r, fmt.Errorf("SendBlob: unpack sendData: %v", err))
 		return
 	}
 
@@ -27,29 +27,29 @@ func (b *blob) Inject(w http.ResponseWriter, r *http.Request, sendData string) {
 
 	sizeOutput, err := gitCommand("", "git", "--git-dir="+params.RepoPath, "cat-file", "-s", params.BlobId).Output()
 	if err != nil {
-		helper.Fail500(w, fmt.Errorf("SendBlob: get blob size: %v", err))
+		helper.Fail500(w, r, fmt.Errorf("SendBlob: get blob size: %v", err))
 		return
 	}
 
 	gitShowCmd := gitCommand("", "git", "--git-dir="+params.RepoPath, "cat-file", "blob", params.BlobId)
 	stdout, err := gitShowCmd.StdoutPipe()
 	if err != nil {
-		helper.Fail500(w, fmt.Errorf("SendBlob: git cat-file stdout: %v", err))
+		helper.Fail500(w, r, fmt.Errorf("SendBlob: git cat-file stdout: %v", err))
 		return
 	}
 	if err := gitShowCmd.Start(); err != nil {
-		helper.Fail500(w, fmt.Errorf("SendBlob: start %v: %v", gitShowCmd, err))
+		helper.Fail500(w, r, fmt.Errorf("SendBlob: start %v: %v", gitShowCmd, err))
 		return
 	}
 	defer helper.CleanUpProcessGroup(gitShowCmd)
 
 	w.Header().Set("Content-Length", strings.TrimSpace(string(sizeOutput)))
 	if _, err := io.Copy(w, stdout); err != nil {
-		helper.LogError(fmt.Errorf("SendBlob: copy git cat-file stdout: %v", err))
+		helper.LogError(r, &copyError{fmt.Errorf("SendBlob: copy git cat-file stdout: %v", err)})
 		return
 	}
 	if err := gitShowCmd.Wait(); err != nil {
-		helper.LogError(fmt.Errorf("SendBlob: wait for git cat-file: %v", err))
+		helper.LogError(r, fmt.Errorf("SendBlob: wait for git cat-file: %v", err))
 		return
 	}
 }

internal/git/diff.go

@@ -22,7 +22,7 @@ var SendDiff = &diff{"git-diff:"}
 func (d *diff) Inject(w http.ResponseWriter, r *http.Request, sendData string) {
 	var params diffParams
 	if err := d.Unpack(&params, sendData); err != nil {
-		helper.Fail500(w, fmt.Errorf("SendDiff: unpack sendData: %v", err))
+		helper.Fail500(w, r, fmt.Errorf("SendDiff: unpack sendData: %v", err))
 		return
 	}
 
@@ -31,23 +31,26 @@ func (d *diff) Inject(w http.ResponseWriter, r *http.Request, sendData string) {
 	gitDiffCmd := gitCommand("", "git", "--git-dir="+params.RepoPath, "diff", params.ShaFrom, params.ShaTo)
 	stdout, err := gitDiffCmd.StdoutPipe()
 	if err != nil {
-		helper.Fail500(w, fmt.Errorf("SendDiff: create stdout pipe: %v", err))
+		helper.Fail500(w, r, fmt.Errorf("SendDiff: create stdout pipe: %v", err))
 		return
 	}
 
 	if err := gitDiffCmd.Start(); err != nil {
-		helper.Fail500(w, fmt.Errorf("SendDiff: start %v: %v", gitDiffCmd, err))
+		helper.Fail500(w, r, fmt.Errorf("SendDiff: start %v: %v", gitDiffCmd.Args, err))
 		return
 	}
 	defer helper.CleanUpProcessGroup(gitDiffCmd)
 
 	w.Header().Del("Content-Length")
 	if _, err := io.Copy(w, stdout); err != nil {
-		helper.LogError(fmt.Errorf("SendDiff: copy %v stdout: %v", gitDiffCmd, err))
+		helper.LogError(
+			r,
+			&copyError{fmt.Errorf("SendDiff: copy %v stdout: %v", gitDiffCmd.Args, err)},
+		)
 		return
 	}
 	if err := gitDiffCmd.Wait(); err != nil {
-		helper.LogError(fmt.Errorf("SendDiff: wait for %v: %v", gitDiffCmd, err))
+		helper.LogError(r, fmt.Errorf("SendDiff: wait for %v: %v", gitDiffCmd.Args, err))
 		return
 	}
 }

internal/git/error.go

+package git
+
+// For cosmetic purposes in Sentry
+type copyError struct{ error }

internal/git/format-patch.go

@@ -22,7 +22,7 @@ var SendPatch = &patch{"git-format-patch:"}
 func (p *patch) Inject(w http.ResponseWriter, r *http.Request, sendData string) {
 	var params patchParams
 	if err := p.Unpack(&params, sendData); err != nil {
-		helper.Fail500(w, fmt.Errorf("SendPatch: unpack sendData: %v", err))
+		helper.Fail500(w, r, fmt.Errorf("SendPatch: unpack sendData: %v", err))
 		return
 	}
 
@@ -33,23 +33,23 @@ func (p *patch) Inject(w http.ResponseWriter, r *http.Request, sendData string)
 
 	stdout, err := gitPatchCmd.StdoutPipe()
 	if err != nil {
-		helper.Fail500(w, fmt.Errorf("SendPatch: create stdout pipe: %v", err))
+		helper.Fail500(w, r, fmt.Errorf("SendPatch: create stdout pipe: %v", err))
 		return
 	}
 
 	if err := gitPatchCmd.Start(); err != nil {
-		helper.Fail500(w, fmt.Errorf("SendPatch: start %v: %v", gitPatchCmd, err))
+		helper.Fail500(w, r, fmt.Errorf("SendPatch: start %v: %v", gitPatchCmd.Args, err))
 		return
 	}
 	defer helper.CleanUpProcessGroup(gitPatchCmd)
 
 	w.Header().Del("Content-Length")
 	if _, err := io.Copy(w, stdout); err != nil {
-		helper.LogError(fmt.Errorf("SendPatch: copy %v stdout: %v", gitPatchCmd, err))
+		helper.LogError(r, &copyError{fmt.Errorf("SendPatch: copy %v stdout: %v", gitPatchCmd.Args, err)})
 		return
 	}
 	if err := gitPatchCmd.Wait(); err != nil {
-		helper.LogError(fmt.Errorf("SendPatch: wait for %v: %v", gitPatchCmd, err))
+		helper.LogError(r, fmt.Errorf("SendPatch: wait for %v: %v", gitPatchCmd.Args, err))
 		return
 	}
 }

internal/git/git-http.go

@@ -5,7 +5,6 @@ In this file we handle the Git 'smart HTTP' protocol
 package git
 
 import (
-	"errors"
 	"fmt"
 	"io"
 	"log"
@@ -40,7 +39,7 @@ func looksLikeRepo(p string) bool {
 func repoPreAuthorizeHandler(myAPI *api.API, handleFunc api.HandleFunc) http.Handler {
 	return myAPI.PreAuthorizeHandler(func(w http.ResponseWriter, r *http.Request, a *api.Response) {
 		if a.RepoPath == "" {
-			helper.Fail500(w, errors.New("repoPreAuthorizeHandler: RepoPath empty"))
+			helper.Fail500(w, r, fmt.Errorf("repoPreAuthorizeHandler: RepoPath empty"))
 			return
 		}
 
@@ -65,12 +64,12 @@ func handleGetInfoRefs(w http.ResponseWriter, r *http.Request, a *api.Response)
 	cmd := gitCommand(a.GL_ID, "git", subCommand(rpc), "--stateless-rpc", "--advertise-refs", a.RepoPath)
 	stdout, err := cmd.StdoutPipe()
 	if err != nil {
-		helper.Fail500(w, fmt.Errorf("handleGetInfoRefs: stdout: %v", err))
+		helper.Fail500(w, r, fmt.Errorf("handleGetInfoRefs: stdout: %v", err))
 		return
 	}
 	defer stdout.Close()
 	if err := cmd.Start(); err != nil {
-		helper.Fail500(w, fmt.Errorf("handleGetInfoRefs: start %v: %v", cmd.Args, err))
+		helper.Fail500(w, r, fmt.Errorf("handleGetInfoRefs: start %v: %v", cmd.Args, err))
 		return
 	}
 	defer helper.CleanUpProcessGroup(cmd) // Ensure brute force subprocess clean-up
@@ -80,19 +79,22 @@ func handleGetInfoRefs(w http.ResponseWriter, r *http.Request, a *api.Response)
 	w.Header().Add("Cache-Control", "no-cache")
 	w.WriteHeader(200) // Don't bother with HTTP 500 from this point on, just return
 	if err := pktLine(w, fmt.Sprintf("# service=%s\n", rpc)); err != nil {
-		helper.LogError(fmt.Errorf("handleGetInfoRefs: pktLine: %v", err))
+		helper.LogError(r, fmt.Errorf("handleGetInfoRefs: pktLine: %v", err))
 		return
 	}
 	if err := pktFlush(w); err != nil {
-		helper.LogError(fmt.Errorf("handleGetInfoRefs: pktFlush: %v", err))
+		helper.LogError(r, fmt.Errorf("handleGetInfoRefs: pktFlush: %v", err))
 		return
 	}
 	if _, err := io.Copy(w, stdout); err != nil {
-		helper.LogError(fmt.Errorf("handleGetInfoRefs: copy output of %v: %v", cmd.Args, err))
+		helper.LogError(
+			r,
+			&copyError{fmt.Errorf("handleGetInfoRefs: copy output of %v: %v", cmd.Args, err)},
+		)
 		return
 	}
 	if err := cmd.Wait(); err != nil {
-		helper.LogError(fmt.Errorf("handleGetInfoRefs: wait for %v: %v", cmd.Args, err))
+		helper.LogError(r, fmt.Errorf("handleGetInfoRefs: wait for %v: %v", cmd.Args, err))
 		return
 	}
 }
@@ -104,7 +106,7 @@ func handlePostRPC(w http.ResponseWriter, r *http.Request, a *api.Response) {
 	action := filepath.Base(r.URL.Path)
 	if !(action == "git-upload-pack" || action == "git-receive-pack") {
 		// The 'dumb' Git HTTP protocol is not supported
-		helper.Fail500(w, fmt.Errorf("handlePostRPC: unsupported action: %s", r.URL.Path))
+		helper.Fail500(w, r, fmt.Errorf("handlePostRPC: unsupported action: %s", r.URL.Path))
 		return
 	}
 
@@ -112,25 +114,25 @@ func handlePostRPC(w http.ResponseWriter, r *http.Request, a *api.Response) {
 	cmd := gitCommand(a.GL_ID, "git", subCommand(action), "--stateless-rpc", a.RepoPath)
 	stdout, err := cmd.StdoutPipe()
 	if err != nil {
-		helper.Fail500(w, fmt.Errorf("handlePostRPC: stdout: %v", err))
+		helper.Fail500(w, r, fmt.Errorf("handlePostRPC: stdout: %v", err))
 		return
 	}
 	defer stdout.Close()
 	stdin, err := cmd.StdinPipe()
 	if err != nil {
-		helper.Fail500(w, fmt.Errorf("handlePostRPC: stdin: %v", err))
+		helper.Fail500(w, r, fmt.Errorf("handlePostRPC: stdin: %v", err))
 		return
 	}
 	defer stdin.Close()
 	if err := cmd.Start(); err != nil {
-		helper.Fail500(w, fmt.Errorf("handlePostRPC: start %v: %v", cmd.Args, err))
+		helper.Fail500(w, r, fmt.Errorf("handlePostRPC: start %v: %v", cmd.Args, err))
 		return
 	}
 	defer helper.CleanUpProcessGroup(cmd) // Ensure brute force subprocess clean-up
 
 	// Write the client request body to Git's standard input
 	if _, err := io.Copy(stdin, r.Body); err != nil {
-		helper.Fail500(w, fmt.Errorf("handlePostRPC write to %v: %v", cmd.Args, err))
+		helper.Fail500(w, r, fmt.Errorf("handlePostRPC: write to %v: %v", cmd.Args, err))
 		return
 	}
 	// Signal to the Git subprocess that no more data is coming
@@ -147,11 +149,14 @@ func handlePostRPC(w http.ResponseWriter, r *http.Request, a *api.Response) {
 
 	// This io.Copy may take a long time, both for Git push and pull.
 	if _, err := io.Copy(w, stdout); err != nil {
-		helper.LogError(fmt.Errorf("handlePostRPC copy output of %v: %v", cmd.Args, err))
+		helper.LogError(
+			r,
+			&copyError{fmt.Errorf("handlePostRPC: copy output of %v: %v", cmd.Args, err)},
+		)
 		return
 	}
 	if err := cmd.Wait(); err != nil {
-		helper.LogError(fmt.Errorf("handlePostRPC wait for %v: %v", cmd.Args, err))
+		helper.LogError(r, fmt.Errorf("handlePostRPC: wait for %v: %v", cmd.Args, err))
 		return
 	}
 }

internal/helper/helpers.go

@@ -8,18 +8,25 @@ import (
 	"os"
 	"os/exec"
 	"syscall"
-
-	"github.com/getsentry/raven-go"
 )
 
-func Fail500(w http.ResponseWriter, err error) {
+func Fail500(w http.ResponseWriter, r *http.Request, err error) {
 	http.Error(w, "Internal server error", 500)
-	LogError(err)
+	captureRavenError(r, err)
+	printError(r, err)
+}
+
+func LogError(r *http.Request, err error) {
+	captureRavenError(r, err)
+	printError(r, err)
 }
 
-func LogError(err error) {
-	raven.CaptureError(err, nil)
-	log.Printf("error: %v", err)
+func printError(r *http.Request, err error) {
+	if r != nil {
+		log.Printf("error: %s %q: %v", r.Method, r.RequestURI, err)
+	} else {
+		log.Printf("error: %v", err)
+	}
 }
 
 func SetNoCacheHeaders(header http.Header) {

internal/helper/raven.go

+package helper
+
+import (
+	"net/http"
+	"reflect"
+
+	"github.com/getsentry/raven-go"
+)
+
+var ravenHeaderBlacklist = []string{
+	"Authorization",
+	"Private-Token",
+}
+
+func captureRavenError(r *http.Request, err error) {
+	client := raven.DefaultClient
+
+	interfaces := []raven.Interface{}
+	if r != nil {
+		CleanHeadersForRaven(r)
+		interfaces = append(interfaces, raven.NewHttp(r))
+	}
+
+	exception := &raven.Exception{
+		Stacktrace: raven.NewStacktrace(2, 3, nil),
+		Value:      err.Error(),
+		Type:       reflect.TypeOf(err).String(),
+	}
+	interfaces = append(interfaces, exception)
+
+	packet := raven.NewPacket(err.Error(), interfaces...)
+	client.Capture(packet, nil)
+}
+
+func CleanHeadersForRaven(r *http.Request) {
+	if r == nil {
+		return
+	}
+
+	for _, key := range ravenHeaderBlacklist {
+		if r.Header.Get(key) != "" {
+			r.Header.Set(key, "[redacted]")
+		}
+	}
+}

internal/lfs/lfs.go

@@ -8,7 +8,6 @@ import (
 	"bytes"
 	"crypto/sha256"
 	"encoding/hex"
-	"errors"
 	"fmt"
 	"io"
 	"io/ioutil"
@@ -28,17 +27,17 @@ func lfsAuthorizeHandler(myAPI *api.API, handleFunc api.HandleFunc) http.Handler
 	return myAPI.PreAuthorizeHandler(func(w http.ResponseWriter, r *http.Request, a *api.Response) {
 
 		if a.StoreLFSPath == "" {
-			helper.Fail500(w, errors.New("lfsAuthorizeHandler: StoreLFSPath empty"))
+			helper.Fail500(w, r, fmt.Errorf("lfsAuthorizeHandler: StoreLFSPath empty"))
 			return
 		}
 
 		if a.LfsOid == "" {
-			helper.Fail500(w, errors.New("lfsAuthorizeHandler: LfsOid empty"))
+			helper.Fail500(w, r, fmt.Errorf("lfsAuthorizeHandler: LfsOid empty"))
 			return
 		}
 
 		if err := os.MkdirAll(a.StoreLFSPath, 0700); err != nil {
-			helper.Fail500(w, fmt.Errorf("lfsAuthorizeHandler: mkdir StoreLFSPath: %v", err))
+			helper.Fail500(w, r, fmt.Errorf("lfsAuthorizeHandler: mkdir StoreLFSPath: %v", err))
 			return
 		}
 
@@ -50,7 +49,7 @@ func handleStoreLfsObject(h http.Handler) api.HandleFunc {
 	return func(w http.ResponseWriter, r *http.Request, a *api.Response) {
 		file, err := ioutil.TempFile(a.StoreLFSPath, a.LfsOid)
 		if err != nil {
-			helper.Fail500(w, fmt.Errorf("handleStoreLfsObject: create tempfile: %v", err))
+			helper.Fail500(w, r, fmt.Errorf("handleStoreLfsObject: create tempfile: %v", err))
 			return
 		}
 		defer os.Remove(file.Name())
@@ -61,19 +60,19 @@ func handleStoreLfsObject(h http.Handler) api.HandleFunc {
 
 		written, err := io.Copy(hw, r.Body)
 		if err != nil {
-			helper.Fail500(w, fmt.Errorf("handleStoreLfsObject: copy body to tempfile: %v", err))
+			helper.Fail500(w, r, fmt.Errorf("handleStoreLfsObject: copy body to tempfile: %v", err))
 			return
 		}
 		file.Close()
 
 		if written != a.LfsSize {
-			helper.Fail500(w, fmt.Errorf("handleStoreLfsObject: expected size %d, wrote %d", a.LfsSize, written))
+			helper.Fail500(w, r, fmt.Errorf("handleStoreLfsObject: expected size %d, wrote %d", a.LfsSize, written))
 			return
 		}
 
 		shaStr := hex.EncodeToString(hash.Sum(nil))
 		if shaStr != a.LfsOid {
-			helper.Fail500(w, fmt.Errorf("handleStoreLfsObject: expected sha256 %s, got %s", a.LfsOid, shaStr))
+			helper.Fail500(w, r, fmt.Errorf("handleStoreLfsObject: expected sha256 %s, got %s", a.LfsOid, shaStr))
 			return
 		}
 

internal/staticpages/servefile.go

@@ -28,7 +28,7 @@ func (s *Static) ServeExisting(prefix urlprefix.Prefix, cache CacheMode, notFoun
 
 		// The filepath.Join does Clean traversing directories up
 		if !strings.HasPrefix(file, s.DocumentRoot) {
-			helper.Fail500(w, &os.PathError{
+			helper.Fail500(w, r, &os.PathError{
 				Op:   "open",
 				Path: file,
 				Err:  os.ErrInvalid,

internal/upload/uploads.go

@@ -104,7 +104,7 @@ func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, te
 
 func HandleFileUploads(w http.ResponseWriter, r *http.Request, h http.Handler, tempPath string, filter MultipartFormProcessor) {
 	if tempPath == "" {
-		helper.Fail500(w, fmt.Errorf("handleFileUploads: tempPath empty"))
+		helper.Fail500(w, r, fmt.Errorf("handleFileUploads: tempPath empty"))
 		return
 	}
 
@@ -118,7 +118,7 @@ func HandleFileUploads(w http.ResponseWriter, r *http.Request, h http.Handler, t
 		if err == http.ErrNotMultipart {
 			h.ServeHTTP(w, r)
 		} else {
-			helper.Fail500(w, fmt.Errorf("handleFileUploads: extract files from multipart: %v", err))
+			helper.Fail500(w, r, fmt.Errorf("handleFileUploads: extract files from multipart: %v", err))
 		}
 		return
 	}

internal/upload/uploads_test.go

@@ -41,7 +41,10 @@ func (a *testFormProcessor) ProcessField(formName string, writer *multipart.Writ
 
 func TestUploadTempPathRequirement(t *testing.T) {
 	response := httptest.NewRecorder()
-	request := &http.Request{}
+	request, err := http.NewRequest("", "", nil)
+	if err != nil {
+		t.Fatal(err)
+	}
 	HandleFileUploads(response, request, nilHandler, "", nil)
 	testhelper.AssertResponseCode(t, response, 500)
 }

internal/upstream/handlers.go

@@ -26,7 +26,7 @@ func contentEncodingHandler(h http.Handler) http.Handler {
 		}
 
 		if err != nil {
-			helper.Fail500(w, fmt.Errorf("contentEncodingHandler: %v", err))
+			helper.Fail500(w, r, fmt.Errorf("contentEncodingHandler: %v", err))
 			return
 		}
 		defer body.Close()

main.go

@@ -25,8 +25,6 @@ import (
 	"time"
 
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/upstream"
-
-	"github.com/getsentry/raven-go"
 )
 
 // Current version of GitLab Workhorse
@@ -91,11 +89,6 @@ func main() {
 		}()
 	}
 
-	// Use a custom environment variable (not SENTRY_DSN) to prevent
-	// clashes with gitlab-rails.
-	raven.SetDSN(os.Getenv("GITLAB_WORKHORSE_SENTRY_DSN"))
-	raven.DefaultClient.SetRelease(Version)
-
 	up := wrapRaven(
 		upstream.NewUpstream(
 			backendURL,
@@ -109,7 +102,3 @@ func main() {
 
 	log.Fatal(http.Serve(listener, up))
 }
-
-func wrapRaven(h http.Handler) http.Handler {
-	return http.HandlerFunc(raven.RecoveryHandler(h.ServeHTTP))
-}

raven.go

+package main
+
+import (
+	"net/http"
+	"os"
+
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
+
+	"github.com/getsentry/raven-go"
+)
+
+func wrapRaven(h http.Handler) http.Handler {
+	// Use a custom environment variable (not SENTRY_DSN) to prevent
+	// clashes with gitlab-rails.
+	sentryDSN := os.Getenv("GITLAB_WORKHORSE_SENTRY_DSN")
+	raven.SetDSN(sentryDSN) // sentryDSN may be empty
+
+	if sentryDSN == "" {
+		return h
+	}
+
+	raven.DefaultClient.SetRelease(Version)
+
+	return http.HandlerFunc(raven.RecoveryHandler(
+		func(w http.ResponseWriter, r *http.Request) {
+			defer func() {
+				if p := recover(); p != nil {
+					helper.CleanHeadersForRaven(r)
+					panic(p)
+				}
+			}()
+
+			h.ServeHTTP(w, r)
+		}))
+}