-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add truncation to body #17723
Add truncation to body #17723
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,6 +32,10 @@ const ( | |
// defaultRetryMax is set to 0 to turn off retry functionality, until dynamic configuration is possible. | ||
// This is to circumvent any spikes in load that may cause or exacerbate server-side issues for now. | ||
defaultRetryMax = 0 | ||
|
||
// defaultErrRespBodyLength refers to the max character length of the body on a failure to export metrics. | ||
// anything beyond we will truncate. | ||
defaultErrRespBodyLength = 100 | ||
) | ||
|
||
// MetricsClient exports Consul metrics in OTLP format to the HCP Telemetry Gateway. | ||
|
@@ -150,8 +154,18 @@ func (o *otlpClient) ExportMetrics(ctx context.Context, protoMetrics *metricpb.R | |
} | ||
|
||
if resp.StatusCode != http.StatusOK { | ||
return fmt.Errorf("failed to export metrics: code %d: %s", resp.StatusCode, string(body)) | ||
truncatedBody := truncate(respData.String(), defaultErrRespBodyLength) | ||
return fmt.Errorf("failed to export metrics: code %d: %s", resp.StatusCode, truncatedBody) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
func truncate(text string, width uint) string { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. width can just be an int? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely but I didn't want to add a check to see if it was less than 0 hence the uint. I can do so if you'd prefer but I though this was more human readable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my take is both could be argued as more readable:
I would probably lean to the int due to familiarity, but its only a nit that I would block on or a hill that I would die on. |
||
if len(text) <= int(width) { | ||
return text | ||
} | ||
r := []rune(text) | ||
trunc := r[:width] | ||
return string(trunc) + "..." | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ package client | |
import ( | ||
"context" | ||
"fmt" | ||
"math/rand" | ||
"net/http" | ||
"net/http/httptest" | ||
"testing" | ||
|
@@ -64,10 +65,21 @@ func TestNewMetricsClient(t *testing.T) { | |
} | ||
} | ||
|
||
var letterRunes = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZäöüÄÖÜ世界") | ||
|
||
func randStringRunes(n int) string { | ||
b := make([]rune, n) | ||
for i := range b { | ||
b[i] = letterRunes[rand.Intn(len(letterRunes))] | ||
} | ||
return string(b) | ||
} | ||
|
||
func TestExportMetrics(t *testing.T) { | ||
for name, test := range map[string]struct { | ||
wantErr string | ||
status int | ||
wantErr string | ||
status int | ||
largeBodyError bool | ||
}{ | ||
"success": { | ||
status: http.StatusOK, | ||
|
@@ -76,8 +88,14 @@ func TestExportMetrics(t *testing.T) { | |
status: http.StatusBadRequest, | ||
wantErr: "failed to export metrics: code 400", | ||
}, | ||
"failsWithNonRetryableErrorWithLongError": { | ||
status: http.StatusBadRequest, | ||
wantErr: "failed to export metrics: code 400", | ||
largeBodyError: true, | ||
}, | ||
} { | ||
t.Run(name, func(t *testing.T) { | ||
randomBody := randStringRunes(1000) | ||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
require.Equal(t, r.Header.Get("content-type"), "application/x-protobuf") | ||
require.Equal(t, r.Header.Get("x-hcp-resource-id"), testResourceID) | ||
|
@@ -91,7 +109,12 @@ func TestExportMetrics(t *testing.T) { | |
|
||
w.Header().Set("Content-Type", "application/x-protobuf") | ||
w.WriteHeader(test.status) | ||
w.Write(bytes) | ||
if test.largeBodyError { | ||
w.Write([]byte(randomBody)) | ||
} else { | ||
w.Write(bytes) | ||
} | ||
|
||
})) | ||
defer srv.Close() | ||
|
||
|
@@ -105,10 +128,48 @@ func TestExportMetrics(t *testing.T) { | |
if test.wantErr != "" { | ||
require.Error(t, err) | ||
require.Contains(t, err.Error(), test.wantErr) | ||
if test.largeBodyError { | ||
truncatedBody := truncate(randomBody, defaultErrRespBodyLength) | ||
require.Contains(t, err.Error(), truncatedBody) | ||
} | ||
return | ||
} | ||
|
||
require.NoError(t, err) | ||
}) | ||
} | ||
} | ||
|
||
func TestTruncate(t *testing.T) { | ||
for name, tc := range map[string]struct { | ||
body string | ||
expectedSize int | ||
}{ | ||
"ZeroSize": { | ||
body: "", | ||
expectedSize: 0, | ||
}, | ||
"LessThanSize": { | ||
body: "foobar", | ||
expectedSize: 6, | ||
}, | ||
"defaultSize": { | ||
body: "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Duis vel tincidunt nunc, sed tristique risu", | ||
expectedSize: 100, | ||
}, | ||
"greaterThanSize": { | ||
body: "Lorem ipsum dolor sit amet, consectetur adipiscing elit. Duis vel tincidunt nunc, sed tristique risus", | ||
expectedSize: 103, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 103 due to ellipsis |
||
}, | ||
"greaterThanSizeWithUnicode": { | ||
body: randStringRunes(1000), | ||
expectedSize: 103, | ||
}, | ||
} { | ||
t.Run(name, func(t *testing.T) { | ||
truncatedBody := truncate(tc.body, defaultErrRespBodyLength) | ||
truncatedRunes := []rune(truncatedBody) | ||
require.Equal(t, len(truncatedRunes), tc.expectedSize) | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note:
body
here is actually a reference to the request body instead of the response body so fixing that up withrespData.String()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops 😓 thanks, good catch!