Skip to content

Commit

Permalink
Add bodyclose linter
Browse files Browse the repository at this point in the history
  • Loading branch information
pellared committed Jul 31, 2024
1 parent a004d84 commit a35b5b8
Show file tree
Hide file tree
Showing 14 changed files with 62 additions and 39 deletions.
1 change: 1 addition & 0 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ linters:
# Specifically enable linters we want to use.
enable:
- asasalint
- bodyclose
- depguard
- errcheck
- errorlint
Expand Down
16 changes: 10 additions & 6 deletions exporters/autoexport/metrics_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (

"github.com/prometheus/client_golang/prometheus"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"go.uber.org/goleak"
"google.golang.org/protobuf/proto"

Expand Down Expand Up @@ -136,9 +137,10 @@ func TestMetricExporterPrometheus(t *testing.T) {
}

resp, err := http.Get(fmt.Sprintf("http://%s/metrics", rws.addr))
assert.NoError(t, err)
require.NoError(t, err)
defer func() { assert.NoError(t, resp.Body.Close()) }()
body, err := io.ReadAll(resp.Body)
assert.NoError(t, err)
require.NoError(t, err)
assert.Contains(t, string(body), "# HELP")

assert.NoError(t, mp.Shutdown(context.Background()))
Expand Down Expand Up @@ -213,9 +215,10 @@ func TestMetricProducerPrometheusWithPrometheusExporter(t *testing.T) {
}

resp, err := http.Get(fmt.Sprintf("http://%s/metrics", rws.addr))
assert.NoError(t, err)
require.NoError(t, err)
defer func() { assert.NoError(t, resp.Body.Close()) }()
body, err := io.ReadAll(resp.Body)
assert.NoError(t, err)
require.NoError(t, err)

// By default there are two metrics exporter. target_info and promhttp_metric_handler_errors_total.
// But by including the prometheus producer we should have more.
Expand Down Expand Up @@ -254,9 +257,10 @@ func TestMetricProducerFallbackWithPrometheusExporter(t *testing.T) {
}

resp, err := http.Get(fmt.Sprintf("http://%s/metrics", rws.addr))
assert.NoError(t, err)
require.NoError(t, err)
defer func() { assert.NoError(t, resp.Body.Close()) }()
body, err := io.ReadAll(resp.Body)
assert.NoError(t, err)
require.NoError(t, err)

assert.Contains(t, string(body), "HELP dummy_metric_total dummy metric")

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ package main
import (
"context"
"encoding/json"
"io"
"log"
"net/http"

Expand Down Expand Up @@ -65,12 +64,12 @@ func lambdaHandler(ctx context.Context) error {
return err
}

defer func(Body io.ReadCloser) {
err := Body.Close()
defer func() {
err := res.Body.Close()
if err != nil {
log.Printf("failed to close http response body, %v\n", err)
}
}(res.Body)
}()

var data map[string]interface{}
err = json.NewDecoder(res.Body).Decode(&data)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ func TestWithPublicEndpoint(t *testing.T) {

rr := httptest.NewRecorder()
container.ServeHTTP(rr, r)
assert.Equal(t, 200, rr.Result().StatusCode)
assert.Equal(t, 200, rr.Result().StatusCode) //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.

// Recorded span should be linked with an incoming span context.
assert.NoError(t, spanRecorder.ForceFlush(ctx))
Expand Down Expand Up @@ -244,7 +244,7 @@ func TestWithPublicEndpointFn(t *testing.T) {

rr := httptest.NewRecorder()
container.ServeHTTP(rr, r)
assert.Equal(t, http.StatusOK, rr.Result().StatusCode)
assert.Equal(t, http.StatusOK, rr.Result().StatusCode) //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.

// Recorded span should be linked with an incoming span context.
assert.NoError(t, spanRecorder.ForceFlush(ctx))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func TestGetSpanNotInstrumented(t *testing.T) {
r := httptest.NewRequest("GET", "/ping", nil)
w := httptest.NewRecorder()
router.ServeHTTP(w, r)
response := w.Result()
response := w.Result() //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.
assert.Equal(t, http.StatusOK, response.StatusCode)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ func TestTrace200(t *testing.T) {

// do and verify the request
router.ServeHTTP(w, r)
response := w.Result()
response := w.Result() //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.
require.Equal(t, http.StatusOK, response.StatusCode)

// verify traces look good
Expand Down Expand Up @@ -109,7 +109,7 @@ func TestError(t *testing.T) {
r := httptest.NewRequest("GET", "/server_err", nil)
w := httptest.NewRecorder()
router.ServeHTTP(w, r)
response := w.Result()
response := w.Result() //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.
assert.Equal(t, http.StatusInternalServerError, response.StatusCode)

// verify the errors and status are correct
Expand Down Expand Up @@ -201,7 +201,7 @@ func TestHTTPRouteWithSpanNameFormatter(t *testing.T) {

// do and verify the request
router.ServeHTTP(w, r)
response := w.Result()
response := w.Result() //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.
require.Equal(t, http.StatusOK, response.StatusCode)

// verify traces look good
Expand Down Expand Up @@ -234,7 +234,7 @@ func TestHTML(t *testing.T) {
r := httptest.NewRequest("GET", "/hello", nil)
w := httptest.NewRecorder()
router.ServeHTTP(w, r)
response := w.Result()
response := w.Result() //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.
assert.Equal(t, http.StatusOK, response.StatusCode)
assert.Equal(t, "hello world", w.Body.String())

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ func TestWithPublicEndpoint(t *testing.T) {
prop.Inject(ctx, propagation.HeaderCarrier(r0.Header))

router.ServeHTTP(w, r0)
assert.Equal(t, http.StatusOK, w.Result().StatusCode)
assert.Equal(t, http.StatusOK, w.Result().StatusCode) //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.

// Recorded span should be linked with an incoming span context.
assert.NoError(t, sr.ForceFlush(ctx))
Expand Down Expand Up @@ -273,7 +273,7 @@ func TestWithPublicEndpointFn(t *testing.T) {
prop.Inject(ctx, propagation.HeaderCarrier(r0.Header))

router.ServeHTTP(w, r0)
assert.Equal(t, http.StatusOK, w.Result().StatusCode)
assert.Equal(t, http.StatusOK, w.Result().StatusCode) //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.

// Recorded span should be linked with an incoming span context.
assert.NoError(t, sr.ForceFlush(ctx))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func TestGetSpanNotInstrumented(t *testing.T) {
r := httptest.NewRequest("GET", "/ping", nil)
w := httptest.NewRecorder()
router.ServeHTTP(w, r)
response := w.Result()
response := w.Result() //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.
assert.Equal(t, http.StatusOK, response.StatusCode)
}

Expand Down Expand Up @@ -65,7 +65,7 @@ func TestPropagationWithGlobalPropagators(t *testing.T) {

router.ServeHTTP(w, r)
otel.SetTextMapPropagator(propagation.NewCompositeTextMapPropagator())
assert.Equal(t, http.StatusOK, w.Result().StatusCode, "should call the 'user' handler")
assert.Equal(t, http.StatusOK, w.Result().StatusCode, "should call the 'user' handler") //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.
}

func TestPropagationWithCustomPropagators(t *testing.T) {
Expand Down Expand Up @@ -95,7 +95,7 @@ func TestPropagationWithCustomPropagators(t *testing.T) {
})

router.ServeHTTP(w, r)
assert.Equal(t, http.StatusOK, w.Result().StatusCode, "should call the 'user' handler")
assert.Equal(t, http.StatusOK, w.Result().StatusCode, "should call the 'user' handler") //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.
}

func TestSkipper(t *testing.T) {
Expand All @@ -116,5 +116,5 @@ func TestSkipper(t *testing.T) {
})

router.ServeHTTP(w, r)
assert.Equal(t, http.StatusOK, w.Result().StatusCode, "should call the 'ping' handler")
assert.Equal(t, http.StatusOK, w.Result().StatusCode, "should call the 'ping' handler") //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func TestChildSpanFromGlobalTracer(t *testing.T) {
w := httptest.NewRecorder()

router.ServeHTTP(w, r)
assert.Equal(t, http.StatusOK, w.Result().StatusCode, "should call the 'user' handler")
assert.Equal(t, http.StatusOK, w.Result().StatusCode, "should call the 'user' handler") //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.
assert.Len(t, sr.Ended(), 1)
}

Expand All @@ -59,7 +59,7 @@ func TestChildSpanFromCustomTracer(t *testing.T) {
w := httptest.NewRecorder()

router.ServeHTTP(w, r)
assert.Equal(t, http.StatusOK, w.Result().StatusCode, "should call the 'user' handler")
assert.Equal(t, http.StatusOK, w.Result().StatusCode, "should call the 'user' handler") //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.
assert.Len(t, sr.Ended(), 1)
}

Expand All @@ -79,7 +79,7 @@ func TestTrace200(t *testing.T) {

// do and verify the request
router.ServeHTTP(w, r)
response := w.Result()
response := w.Result() //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.
require.Equal(t, http.StatusOK, response.StatusCode)

// verify traces look good
Expand Down Expand Up @@ -111,7 +111,7 @@ func TestError(t *testing.T) {
r := httptest.NewRequest("GET", "/server_err", nil)
w := httptest.NewRecorder()
router.ServeHTTP(w, r)
response := w.Result()
response := w.Result() //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.
assert.Equal(t, http.StatusInternalServerError, response.StatusCode)

// verify the errors and status are correct
Expand Down
2 changes: 1 addition & 1 deletion instrumentation/net/http/otelhttp/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ func TestHandler(t *testing.T) {

rr := httptest.NewRecorder()
tc.handler(t).ServeHTTP(rr, r)
assert.Equal(t, tc.expectedStatusCode, rr.Result().StatusCode)
assert.Equal(t, tc.expectedStatusCode, rr.Result().StatusCode) //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.
})
}
}
6 changes: 3 additions & 3 deletions instrumentation/net/http/otelhttp/test/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func TestBasicFilter(t *testing.T) {
t.Fatal(err)
}
h.ServeHTTP(rr, r)
if got, expected := rr.Result().StatusCode, http.StatusOK; got != expected {
if got, expected := rr.Result().StatusCode, http.StatusOK; got != expected { //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.
t.Fatalf("got %d, expected %d", got, expected)
}
if got := rr.Header().Get("Traceparent"); got != "" {
Expand All @@ -48,7 +48,7 @@ func TestBasicFilter(t *testing.T) {
if got, expected := len(spanRecorder.Ended()), 0; got != expected {
t.Fatalf("got %d recorded spans, expected %d", got, expected)
}
d, err := io.ReadAll(rr.Result().Body)
d, err := io.ReadAll(rr.Result().Body) //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -111,7 +111,7 @@ func TestSpanNameFormatter(t *testing.T) {
t.Fatal(err)
}
h.ServeHTTP(rr, r)
if got, expected := rr.Result().StatusCode, http.StatusOK; got != expected {
if got, expected := rr.Result().StatusCode, http.StatusOK; got != expected { //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.
t.Fatalf("got %d, expected %d", got, expected)
}

Expand Down
10 changes: 5 additions & 5 deletions instrumentation/net/http/otelhttp/test/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ func TestHandlerBasics(t *testing.T) {
)
assertScopeMetrics(t, rm.ScopeMetrics[0], attrs)

if got, expected := rr.Result().StatusCode, http.StatusOK; got != expected {
if got, expected := rr.Result().StatusCode, http.StatusOK; got != expected { //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.
t.Fatalf("got %d, expected %d", got, expected)
}

Expand All @@ -130,7 +130,7 @@ func TestHandlerBasics(t *testing.T) {
t.Fatalf("invalid span created: %#v", spans[0].SpanContext())
}

d, err := io.ReadAll(rr.Result().Body)
d, err := io.ReadAll(rr.Result().Body) //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -291,7 +291,7 @@ func TestHandlerRequestWithTraceContext(t *testing.T) {
r = r.WithContext(ctx)

h.ServeHTTP(rr, r)
assert.Equal(t, http.StatusOK, rr.Result().StatusCode)
assert.Equal(t, http.StatusOK, rr.Result().StatusCode) //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.

span.End()

Expand Down Expand Up @@ -339,7 +339,7 @@ func TestWithPublicEndpoint(t *testing.T) {

rr := httptest.NewRecorder()
h.ServeHTTP(rr, r)
assert.Equal(t, http.StatusOK, rr.Result().StatusCode)
assert.Equal(t, http.StatusOK, rr.Result().StatusCode) //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.

// Recorded span should be linked with an incoming span context.
assert.NoError(t, spanRecorder.ForceFlush(ctx))
Expand Down Expand Up @@ -423,7 +423,7 @@ func TestWithPublicEndpointFn(t *testing.T) {

rr := httptest.NewRecorder()
h.ServeHTTP(rr, r)
assert.Equal(t, http.StatusOK, rr.Result().StatusCode)
assert.Equal(t, http.StatusOK, rr.Result().StatusCode) //nolint:bodyclose // False positive for httptest.ResponseRecorder: https://github.com/timakin/bodyclose/issues/59.

// Recorded span should be linked with an incoming span context.
assert.NoError(t, spanRecorder.ForceFlush(ctx))
Expand Down
15 changes: 12 additions & 3 deletions instrumentation/net/http/otelhttp/test/transport_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,15 @@ func TestTransportErrorStatus(t *testing.T) {
if err != nil {
t.Fatal(err)
}
_, err = c.Do(r)
resp, err := c.Do(r)
if err == nil {
t.Fatal("transport should have returned an error, it didn't")
}
defer func() {
if err := resp.Body.Close(); err != nil {
t.Errorf("close response body: %v", err)
}
}()

// Check span.
spans := spanRecorder.Ended()
Expand Down Expand Up @@ -160,6 +165,7 @@ func TestTransportRequestWithTraceContext(t *testing.T) {
c := http.Client{Transport: tr}
res, err := c.Do(r)
require.NoError(t, err)
defer func() { assert.NoError(t, res.Body.Close()) }()

span.End()

Expand Down Expand Up @@ -220,6 +226,7 @@ func TestWithHTTPTrace(t *testing.T) {
c := http.Client{Transport: tr}
res, err := c.Do(r)
require.NoError(t, err)
defer func() { assert.NoError(t, res.Body.Close()) }()

span.End()

Expand Down Expand Up @@ -513,8 +520,9 @@ func TestCustomAttributesHandling(t *testing.T) {
// test bonus: intententionally ignoring response to confirm that
// http.client.response.size metric is not recorded
// by the Transport.RoundTrip logic
_, err = client.Do(r)
resp, err := client.Do(r)
require.NoError(t, err)
defer func() { assert.NoError(t, resp.Body.Close()) }()

err = reader.Collect(ctx, &rm)
assert.NoError(t, err)
Expand Down Expand Up @@ -587,7 +595,8 @@ func BenchmarkTransportRoundTrip(b *testing.B) {
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
_, _ = c.Do(r)
resp, _ := c.Do(r)
resp.Body.Close()
}
})
}
Expand Down
10 changes: 10 additions & 0 deletions instrumentation/net/http/otelhttp/transport_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,11 @@ func TestTransportBasics(t *testing.T) {
if err != nil {
t.Fatal(err)
}
defer func() {
if err := res.Body.Close(); err != nil {
t.Errorf("close response body: %v", err)
}
}()

body, err := io.ReadAll(res.Body)
if err != nil {
Expand Down Expand Up @@ -170,6 +175,11 @@ func TestNilTransport(t *testing.T) {
if err != nil {
t.Fatal(err)
}
defer func() {
if err := res.Body.Close(); err != nil {
t.Errorf("close response body: %v", err)
}
}()

body, err := io.ReadAll(res.Body)
if err != nil {
Expand Down

0 comments on commit a35b5b8

Please sign in to comment.