From dd6e4dd90cbc1a050cda94e3d88873d0aef05383 Mon Sep 17 00:00:00 2001 From: Christian Stewart Date: Sat, 27 Jul 2024 01:18:05 -0700 Subject: [PATCH] refactor: move http/log to util/httplog Signed-off-by: Christian Stewart --- go.mod | 2 +- go.sum | 4 +- http/log/client.go | 144 ------------------------------- http/log/client_test.go | 47 ---------- http/log/fetch/fetch.go | 1 - http/log/fetch/fetch_js.go | 80 ----------------- http/log/fetch/fetch_test.go | 90 ------------------- http/log/server.go | 53 ------------ http/log/server_test.go | 48 ----------- transport/websocket/websocket.go | 2 +- 10 files changed, 4 insertions(+), 467 deletions(-) delete mode 100644 http/log/client.go delete mode 100644 http/log/client_test.go delete mode 100644 http/log/fetch/fetch.go delete mode 100644 http/log/fetch/fetch_js.go delete mode 100644 http/log/fetch/fetch_test.go delete mode 100644 http/log/server.go delete mode 100644 http/log/server_test.go diff --git a/go.mod b/go.mod index 8802e6c5..06c1c769 100644 --- a/go.mod +++ b/go.mod @@ -8,7 +8,7 @@ require ( github.com/aperturerobotics/entitygraph v0.9.1 // latest github.com/aperturerobotics/protobuf-go-lite v0.6.5 // latest github.com/aperturerobotics/starpc v0.33.6 // latest - github.com/aperturerobotics/util v1.25.1 // latest + github.com/aperturerobotics/util v1.25.2 // latest ) // aperture: use compatibility forks diff --git a/go.sum b/go.sum index 345a7b03..d0535a8f 100644 --- a/go.sum +++ b/go.sum @@ -28,8 +28,8 @@ github.com/aperturerobotics/quic-go v0.45.1-0.20240701204210-82dc570e7aa0 h1:KH1 github.com/aperturerobotics/quic-go v0.45.1-0.20240701204210-82dc570e7aa0/go.mod h1:X095EBMI8M7riYQRvUgegHFkEkgM2QKLvyGHyAcOw/Q= github.com/aperturerobotics/starpc v0.33.6 h1:noc/MnmIMTek9bdEvd88QiD1p9KzEV8CUOBIoKmGgm0= github.com/aperturerobotics/starpc v0.33.6/go.mod h1:4IYcbulEzqhPT5jKaDeL1BJPFd8WVWZ7Ugu0/348/Is= -github.com/aperturerobotics/util v1.25.1 h1:LOIygQIpwBNPwQDWcVT0MPuJxhJsPhPyO/YTJINy83A= -github.com/aperturerobotics/util v1.25.1/go.mod h1:m/paprtgaTiGfc4X3LkXpeseK9hfQA7QBI3cKsE/h3Y= +github.com/aperturerobotics/util v1.25.2 h1:VaP+wLtb28wP1zlwd6idN77tNZJTHTJwswraHsBPJjg= +github.com/aperturerobotics/util v1.25.2/go.mod h1:m/paprtgaTiGfc4X3LkXpeseK9hfQA7QBI3cKsE/h3Y= github.com/blang/semver v3.5.1+incompatible h1:cQNTCjp13qL8KC3Nbxr/y2Bqb63oX6wdnnjpJbkM4JQ= github.com/blang/semver v3.5.1+incompatible/go.mod h1:kRBLl5iJ+tD4TcOOxsy/0fnwebNt5EWlYSAyrTnjyyk= github.com/cenkalti/backoff v2.2.1+incompatible h1:tNowT99t7UNflLxfYYSlKYsBpXdEet03Pg2g16Swow4= diff --git a/http/log/client.go b/http/log/client.go deleted file mode 100644 index 6852c95e..00000000 --- a/http/log/client.go +++ /dev/null @@ -1,144 +0,0 @@ -package httplog - -import ( - "net/http" - "time" - - "github.com/sirupsen/logrus" -) - -// DoRequest performs a request with logging. -// -// If verbose=true, logs successful cases as well as errors. -// le can be nil to disable logging -func DoRequest(le *logrus.Entry, client *http.Client, req *http.Request, verbose bool) (*http.Response, error) { - return DoRequestWithClient(le, client, req, verbose) -} - -// roundTripperClient converts http.RoundTripper to HttpClient. -type roundTripperClient struct { - rt http.RoundTripper -} - -// Do performs the request. -func (r *roundTripperClient) Do(req *http.Request) (*http.Response, error) { - return r.rt.RoundTrip(req) -} - -// _ is a type assertion -var _ HttpClient = (*roundTripperClient)(nil) - -// DoRequestWithTransport performs a request with logging. -// -// If verbose=true, logs successful cases as well as errors. -// le can be nil to disable logging -func DoRequestWithTransport(le *logrus.Entry, transport http.RoundTripper, req *http.Request, verbose bool) (*http.Response, error) { - return DoRequestWithClient(le, &roundTripperClient{rt: transport}, req, verbose) -} - -// loggedClient wraps http.Client to HttpClient with a logger. -type loggedClient struct { - client HttpClient - le *logrus.Entry - verbose bool -} - -// Do performs the request. -func (l *loggedClient) Do(req *http.Request) (*http.Response, error) { - return DoRequestWithClient(l.le, l.client, req, l.verbose) -} - -// _ is a type assertion -var _ HttpClient = (*loggedClient)(nil) - -// NewLoggedClient wraps an HttpClient with a logger. -func NewLoggedClient(le *logrus.Entry, client HttpClient, verbose bool) HttpClient { - return &loggedClient{ - client: client, - le: le, - verbose: verbose, - } -} - -// HttpClient can perform http requests. -type HttpClient interface { - Do(req *http.Request) (*http.Response, error) -} - -// DoRequestWithClient performs a request with logging. -// -// If verbose=true, logs successful cases as well as errors. -// le can be nil to disable logging -func DoRequestWithClient(le *logrus.Entry, client HttpClient, req *http.Request, verbose bool) (*http.Response, error) { - // Request details - if le != nil { - le = le.WithFields(logrus.Fields{ - "method": req.Method, - "url": req.URL.String(), - }) - - // Parse and log the Range header from the request - if rangeHeader := req.Header.Get("Range"); rangeHeader != "" { - le = le.WithField("range", rangeHeader) - } - - if verbose { - le.Debug("starting request") - } - } - - var resp *http.Response - var err error - startTime := time.Now() - if client != nil { - resp, err = client.Do(req) - } else { - resp, err = http.DefaultClient.Do(req) - } - duration := time.Since(startTime) - - if le != nil { - fields := make(logrus.Fields, 3) - fields["dur"] = duration.String() - if resp != nil { - fields["status"] = resp.StatusCode - - // Parse and log the Content-Range header from the response - if contentRangeHeader := resp.Header.Get("Content-Range"); contentRangeHeader != "" { - fields["response-range"] = contentRangeHeader - } - } - - le := le.WithFields(fields) - if err != nil { - le.WithError(err).Warn("request errored") - } else if resp == nil || resp.StatusCode >= 400 { - le.Warn("request failed") - } else if verbose { - le.Debug("request succeeded") - } - } - - return resp, err -} - -// LoggedRoundTripper is a custom RoundTripper that wraps an existing RoundTripper with a logger. -type LoggedRoundTripper struct { - transport http.RoundTripper - le *logrus.Entry - verbose bool -} - -// NewLoggedRoundTripper creates a new instance of LoggedRoundTripper. -func NewLoggedRoundTripper(transport http.RoundTripper, le *logrus.Entry, verbose bool) *LoggedRoundTripper { - return &LoggedRoundTripper{ - transport: transport, - le: le, - verbose: verbose, - } -} - -// RoundTrip implements the RoundTripper interface. -func (t *LoggedRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { - return DoRequestWithTransport(t.le, t.transport, req, t.verbose) -} diff --git a/http/log/client_test.go b/http/log/client_test.go deleted file mode 100644 index c8d602c8..00000000 --- a/http/log/client_test.go +++ /dev/null @@ -1,47 +0,0 @@ -//go:build !js - -package httplog - -import ( - "net/http" - "net/http/httptest" - "testing" - - "github.com/sirupsen/logrus" -) - -func TestDoRequest(t *testing.T) { - logger := logrus.New() - logger.SetLevel(logrus.DebugLevel) - entry := logrus.NewEntry(logger) - - // Create a test server that responds with 200 OK - ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) - defer ts.Close() - - client := &http.Client{} - req, _ := http.NewRequest("GET", ts.URL, nil) - resp, err := DoRequest(entry, client, req, true) - if err != nil { - t.Errorf("Expected no error, got %v", err) - } - if resp.StatusCode != http.StatusOK { - t.Errorf("Expected status 200, got %v", resp.StatusCode) - } - - // Create a test server that responds with 500 Internal Server Error - ts2 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - http.Error(w, "Internal Server Error", http.StatusInternalServerError) - })) - defer ts2.Close() - - req2, _ := http.NewRequest("GET", ts2.URL, nil) - resp2, err2 := DoRequest(entry, client, req2, true) - - if err2 != nil { - t.Errorf("Expected no error, got %v", err2) - } - if resp2.StatusCode != http.StatusInternalServerError { - t.Errorf("Expected status 500, got %v", resp2.StatusCode) - } -} diff --git a/http/log/fetch/fetch.go b/http/log/fetch/fetch.go deleted file mode 100644 index 4a83e5bc..00000000 --- a/http/log/fetch/fetch.go +++ /dev/null @@ -1 +0,0 @@ -package httplog_fetch diff --git a/http/log/fetch/fetch_js.go b/http/log/fetch/fetch_js.go deleted file mode 100644 index b26e5376..00000000 --- a/http/log/fetch/fetch_js.go +++ /dev/null @@ -1,80 +0,0 @@ -//go:build js - -package httplog_fetch - -import ( - "net/textproto" - "slices" - "time" - - fetch "github.com/aperturerobotics/util/js/fetch" - "github.com/sirupsen/logrus" -) - -// logHeaders is the set of headers to attach to the logger as fields. -var logHeaders = []string{"range", "content-range", "content-type", "content-length", "accept"} - -// Fetch uses the JS Fetch API to make requests with logging. -// -// if le is nil, all logging is disabled. -// if verbose is set, both successful and failed calls are logged. -func Fetch(le *logrus.Entry, url string, opts *fetch.Opts, verbose bool) (*fetch.Response, error) { - // Request details - if le != nil { - method := "GET" - if opts != nil && opts.Method != "" { - method = opts.Method - } - - le = le.WithFields(logrus.Fields{ - "method": method, - "url": url, - }) - - if opts != nil && opts.Header != nil { - // Parse and log some headers from the request - for hdr, hdrVal := range opts.Header { - hdr = fetch.CanonicalHeaderKey(hdr) - if slices.Contains(logHeaders, hdr) { - le = le.WithField(hdr, hdrVal) - } - } - } - - if verbose { - le.Debug("starting request") - } - } - - startTime := time.Now() - resp, err := fetch.Fetch(url, opts) - duration := time.Since(startTime) - - if le != nil { - mapSize := 1 - if resp != nil { - mapSize += 1 + min(len(resp.Header), len(logHeaders)) - } - fields := make(logrus.Fields, mapSize) - fields["dur"] = duration.String() - if resp != nil { - fields["status"] = resp.Status - for hdr, hdrVal := range resp.Header { - hdr = textproto.CanonicalMIMEHeaderKey(hdr) - if slices.Contains(logHeaders, hdr) { - fields[hdr] = hdrVal - } - } - } - - if err != nil { - le.WithError(err).Warn("request errored") - } else if resp == nil || resp.StatusCode >= 400 { - le.Warn("request failed") - } else if verbose { - le.Debug("request succeeded") - } - } - - return resp, err -} diff --git a/http/log/fetch/fetch_test.go b/http/log/fetch/fetch_test.go deleted file mode 100644 index 65d01470..00000000 --- a/http/log/fetch/fetch_test.go +++ /dev/null @@ -1,90 +0,0 @@ -//go:build js && webtests - -package httplog_fetch - -import ( - "bytes" - "net/http" - "testing" - - fetch "github.com/aperturerobotics/util/js/fetch" - "github.com/sirupsen/logrus" -) - -func TestFetch(t *testing.T) { - // Create a logger - logger := logrus.New() - logger.SetLevel(logrus.DebugLevel) - - // Test cases - testCases := []struct { - name string - url string - opts *fetch.Opts - verbose bool - expectError bool - }{ - { - name: "Successful GET request", - url: "https://httpbin.org/get", - verbose: true, - }, - { - name: "POST request with headers", - url: "https://httpbin.org/post", - opts: &fetch.Opts{ - Method: "POST", - Header: map[string][]string{ - "Content-Type": []string{"application/json"}, - }, - Body: bytes.NewReader([]byte(`{"test": "data"}`)), - }, - verbose: true, - }, - { - name: "Non-existent URL", - url: "https://thisurldoesnotexist.example.com", - expectError: true, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - le := logger.WithField("test", tc.name) - - resp, err := Fetch(le, tc.url, tc.opts, tc.verbose) - - if tc.expectError { - if err == nil { - t.Errorf("Expected an error, but got nil") - } - if resp != nil { - t.Errorf("Expected nil response, but got %v", resp) - } - } else { - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } - if resp == nil { - t.Fatalf("Expected non-nil response, but got nil") - } - if resp.StatusCode != http.StatusOK { - t.Errorf("Expected status %d, but got %v", http.StatusOK, resp.StatusCode) - } - } - }) - } -} - -func TestFetchWithNilLogger(t *testing.T) { - resp, err := Fetch(nil, "https://httpbin.org/get", nil, false) - if err != nil { - t.Fatalf("Unexpected error: %v", err) - } - if resp == nil { - t.Fatal("Expected non-nil response, but got nil") - } - if resp.StatusCode != http.StatusOK { - t.Errorf("Expected status %d, but got %d", http.StatusOK, resp.StatusCode) - } -} diff --git a/http/log/server.go b/http/log/server.go deleted file mode 100644 index 23d23185..00000000 --- a/http/log/server.go +++ /dev/null @@ -1,53 +0,0 @@ -package httplog - -import ( - "net/http" - - "github.com/sirupsen/logrus" -) - -// LoggingMiddlewareOpts are opts passed to LoggingMiddleware. -type LoggingMiddlewareOpts struct { - // UserAgent includes user agent in logs. - UserAgent bool -} - -// LoggingMiddleware logs incoming requests and response status codes using logrus. -func LoggingMiddleware(next http.Handler, le *logrus.Entry, opts LoggingMiddlewareOpts) http.Handler { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - // Wrap the response writer to capture the status code - wrappedWriter := &statusCapturingResponseWriter{ResponseWriter: w} - - // Call the next handler - next.ServeHTTP(wrappedWriter, r) - - // Log the request and response status code - WithLoggerFields(le, r, wrappedWriter.statusCode). - Debug("handled request") - }) -} - -// WithLoggerFields builds the log fields for a request. -func WithLoggerFields(le *logrus.Entry, r *http.Request, status int) *logrus.Entry { - fields := logrus.Fields{ - "method": r.Method, - "uri": r.RequestURI, - } - if userAgent := r.UserAgent(); userAgent != "" { - fields["user-agent"] = userAgent - } - if status != 0 { - fields["status"] = status - } - return le.WithFields(fields) -} - -type statusCapturingResponseWriter struct { - http.ResponseWriter - statusCode int -} - -func (w *statusCapturingResponseWriter) WriteHeader(statusCode int) { - w.ResponseWriter.WriteHeader(statusCode) - w.statusCode = statusCode -} diff --git a/http/log/server_test.go b/http/log/server_test.go deleted file mode 100644 index 2873af65..00000000 --- a/http/log/server_test.go +++ /dev/null @@ -1,48 +0,0 @@ -//go:build !js - -package httplog - -import ( - "io" - "net/http" - "net/http/httptest" - "net/url" - "testing" - - "github.com/sirupsen/logrus" -) - -// TestHTTPLogServer tests logging http requests to logrus. -func TestHTTPLogServer(t *testing.T) { - log := logrus.New() - log.SetLevel(logrus.DebugLevel) - le := logrus.NewEntry(log) - - handler := http.NewServeMux() - handler.HandleFunc("/test", func(rw http.ResponseWriter, req *http.Request) { - rw.WriteHeader(200) - _, _ = rw.Write([]byte("hello world!\n")) - }) - - srv := httptest.NewServer(LoggingMiddleware(handler, le, LoggingMiddlewareOpts{UserAgent: true})) - defer srv.Close() - baseURL, _ := url.Parse(srv.URL) - baseURL = baseURL.JoinPath("test") - - // Create the client - client := srv.Client() - - // Get the test endpoint - resp, err := client.Get(baseURL.String()) - if err != nil { - t.Fatal(err.Error()) - } - - data, err := io.ReadAll(resp.Body) - if err != nil { - t.Fatal(err.Error()) - } - if string(data) != "hello world!\n" || resp.StatusCode != 200 { - t.Fail() - } -} diff --git a/transport/websocket/websocket.go b/transport/websocket/websocket.go index 9095df9b..0faed6d8 100644 --- a/transport/websocket/websocket.go +++ b/transport/websocket/websocket.go @@ -7,7 +7,7 @@ import ( "strings" "time" - httplog "github.com/aperturerobotics/bifrost/http/log" + httplog "github.com/aperturerobotics/util/httplog" "github.com/aperturerobotics/bifrost/peer" "github.com/aperturerobotics/bifrost/transport" "github.com/aperturerobotics/bifrost/transport/common/dialer"