Skip to content

Commit 162afaa

Browse files
mark-pictor-csecmark-pictor-csec
committed
feedback
1 parent ffa1610 commit 162afaa

File tree

5 files changed

+54
-109
lines changed

5 files changed

+54
-109
lines changed

‎exporters/otlp/otlplog/otlploghttp/client.go

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -197,28 +197,21 @@ func (c *httpClient) uploadLogs(ctx context.Context, data []*logpb.ResourceLogs)
197197
return err
198198
}
199199
respStr := strings.TrimSpace(respData.String())
200+
if len(respStr) == 0 {
201+
respStr = "(empty)"
202+
}
203+
bodyErr := fmt.Errorf("body: %s", respStr)
200204

201205
switch resp.StatusCode {
202206
case http.StatusTooManyRequests,
203207
http.StatusBadGateway,
204208
http.StatusServiceUnavailable,
205209
http.StatusGatewayTimeout:
206210
// Retryable failure.
207-
208-
var err error
209-
if len(respStr) > 0 {
210-
// include response body for context
211-
err = errors.New(respStr)
212-
}
213-
return newResponseError(resp.Header, err)
211+
return newResponseError(resp.Header, bodyErr)
214212
default:
215213
// Non-retryable failure.
216-
if len(respStr) > 0 {
217-
// include response body for context
218-
err = errors.New(respStr)
219-
return fmt.Errorf("failed to send logs to %s: %s (%w)", request.URL, resp.Status, err)
220-
}
221-
return fmt.Errorf("failed to send logs to %s: %s", request.URL, resp.Status)
214+
return fmt.Errorf("failed to send logs to %s: %s (%w)", request.URL, resp.Status, bodyErr)
222215
}
223216
})
224217
}

‎exporters/otlp/otlplog/otlploghttp/client_test.go

Lines changed: 17 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -778,40 +778,24 @@ func TestConfig(t *testing.T) {
778778
require.Contains(t, got, headerKeySetInProxy)
779779
assert.Equal(t, []string{headerValueSetInProxy}, got[headerKeySetInProxy])
780780
})
781-
}
782-
783-
// borrows from TestConfig.
784-
func TestNonRetryable(t *testing.T) {
785-
factoryFunc := func(ePt string, rCh <-chan exportResult, o ...Option) (log.Exporter, *httpCollector) {
786-
coll, err := newHTTPCollector(ePt, rCh)
787-
require.NoError(t, err)
788781

789-
opts := []Option{WithEndpoint(coll.Addr().String())}
790-
if !strings.HasPrefix(strings.ToLower(ePt), "https") {
791-
opts = append(opts, WithInsecure())
792-
}
793-
opts = append(opts, o...)
782+
t.Run("non-retryable errors are propagated", func(t *testing.T) {
783+
exporterErr := errors.New("missing required attribute aaaa")
784+
rCh := make(chan exportResult, 1)
785+
rCh <- exportResult{Err: &httpResponseError{
786+
Status: http.StatusBadRequest,
787+
Err: exporterErr,
788+
}}
794789

790+
exp, coll := factoryFunc("", rCh, WithRetry(RetryConfig{
791+
Enabled: false,
792+
}))
795793
ctx := context.Background()
796-
exp, err := New(ctx, opts...)
797-
require.NoError(t, err)
798-
return exp, coll
799-
}
800-
exporterErr := errors.New("missing required attribute aaaa")
801-
rCh := make(chan exportResult, 1)
802-
rCh <- exportResult{Err: &httpResponseError{
803-
Status: http.StatusBadRequest,
804-
Err: exporterErr,
805-
}}
806-
807-
exp, coll := factoryFunc("", rCh, WithRetry(RetryConfig{
808-
Enabled: false,
809-
}))
810-
ctx := context.Background()
811-
t.Cleanup(func() { require.NoError(t, coll.Shutdown(ctx)) })
812-
// Push this after Shutdown so the HTTP server doesn't hang.
813-
t.Cleanup(func() { close(rCh) })
814-
t.Cleanup(func() { require.NoError(t, exp.Shutdown(ctx)) })
815-
err := exp.Export(ctx, make([]log.Record, 1))
816-
assert.ErrorContains(t, err, exporterErr.Error())
794+
t.Cleanup(func() { require.NoError(t, coll.Shutdown(ctx)) })
795+
// Push this after Shutdown so the HTTP server doesn't hang.
796+
t.Cleanup(func() { close(rCh) })
797+
t.Cleanup(func() { require.NoError(t, exp.Shutdown(ctx)) })
798+
err := exp.Export(ctx, make([]log.Record, 1))
799+
assert.ErrorContains(t, err, exporterErr.Error())
800+
})
817801
}

‎exporters/otlp/otlpmetric/otlpmetrichttp/client.go

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -200,29 +200,21 @@ func (c *client) UploadMetrics(ctx context.Context, protoMetrics *metricpb.Resou
200200
return err
201201
}
202202
respStr := strings.TrimSpace(respData.String())
203+
if len(respStr) == 0 {
204+
respStr = "(empty)"
205+
}
206+
bodyErr := fmt.Errorf("body: %s", respStr)
203207

204208
switch resp.StatusCode {
205209
case http.StatusTooManyRequests,
206210
http.StatusBadGateway,
207211
http.StatusServiceUnavailable,
208212
http.StatusGatewayTimeout:
209213
// Retryable failure.
210-
211-
var err error
212-
if len(respStr) > 0 {
213-
// include response body for context
214-
err = errors.New(respStr)
215-
}
216-
return newResponseError(resp.Header, err)
214+
return newResponseError(resp.Header, bodyErr)
217215
default:
218216
// Non-retryable failure.
219-
220-
if len(respStr) > 0 {
221-
// include response body for context
222-
e := errors.New(respStr)
223-
return fmt.Errorf("failed to send metrics to %s: %s (%w)", request.URL, resp.Status, e)
224-
}
225-
return fmt.Errorf("failed to send metrics to %s: %s", request.URL, resp.Status)
217+
return fmt.Errorf("failed to send metrics to %s: %s (%w)", request.URL, resp.Status, bodyErr)
226218
}
227219
})
228220
}

‎exporters/otlp/otlpmetric/otlpmetrichttp/client_test.go

Lines changed: 19 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -270,41 +270,25 @@ func TestConfig(t *testing.T) {
270270
require.Contains(t, got, headerKeySetInProxy)
271271
assert.Equal(t, []string{headerValueSetInProxy}, got[headerKeySetInProxy])
272272
})
273-
}
274-
275-
// borrows from TestConfig.
276-
func TestNonRetryable(t *testing.T) {
277-
factoryFunc := func(ePt string, rCh <-chan otest.ExportResult, o ...Option) (metric.Exporter, *otest.HTTPCollector) {
278-
coll, err := otest.NewHTTPCollector(ePt, rCh)
279-
require.NoError(t, err)
280-
281-
opts := []Option{WithEndpoint(coll.Addr().String())}
282-
if !strings.HasPrefix(strings.ToLower(ePt), "https") {
283-
opts = append(opts, WithInsecure())
284-
}
285-
opts = append(opts, o...)
286273

274+
t.Run("non-retryable errors are propagated", func(t *testing.T) {
275+
exporterErr := errors.New("missing required attribute aaa")
276+
rCh := make(chan otest.ExportResult, 1)
277+
rCh <- otest.ExportResult{Err: &otest.HTTPResponseError{
278+
Status: http.StatusBadRequest,
279+
Err: exporterErr,
280+
}}
281+
exp, coll := factoryFunc("", rCh)
287282
ctx := context.Background()
288-
exp, err := New(ctx, opts...)
289-
require.NoError(t, err)
290-
return exp, coll
291-
}
292-
exporterErr := errors.New("missing required attribute aaa")
293-
rCh := make(chan otest.ExportResult, 1)
294-
rCh <- otest.ExportResult{Err: &otest.HTTPResponseError{
295-
Status: http.StatusBadRequest,
296-
Err: exporterErr,
297-
}}
298-
exp, coll := factoryFunc("", rCh)
299-
ctx := context.Background()
300-
t.Cleanup(func() { require.NoError(t, coll.Shutdown(ctx)) })
301-
// Push this after Shutdown so the HTTP server doesn't hang.
302-
t.Cleanup(func() { close(rCh) })
303-
t.Cleanup(func() { require.NoError(t, exp.Shutdown(ctx)) })
304-
exCtx, cancel := context.WithTimeout(ctx, time.Second)
305-
defer cancel()
306-
err := exp.Export(exCtx, &metricdata.ResourceMetrics{})
307-
assert.ErrorContains(t, err, exporterErr.Error())
308-
309-
assert.NoError(t, exCtx.Err())
283+
t.Cleanup(func() { require.NoError(t, coll.Shutdown(ctx)) })
284+
// Push this after Shutdown so the HTTP server doesn't hang.
285+
t.Cleanup(func() { close(rCh) })
286+
t.Cleanup(func() { require.NoError(t, exp.Shutdown(ctx)) })
287+
exCtx, cancel := context.WithTimeout(ctx, time.Second)
288+
defer cancel()
289+
err := exp.Export(exCtx, &metricdata.ResourceMetrics{})
290+
assert.ErrorContains(t, err, exporterErr.Error())
291+
292+
assert.NoError(t, exCtx.Err())
293+
})
310294
}

‎exporters/otlp/otlptrace/otlptracehttp/client.go

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -205,29 +205,21 @@ func (d *client) UploadTraces(ctx context.Context, protoSpans []*tracepb.Resourc
205205
return err
206206
}
207207
respStr := strings.TrimSpace(respData.String())
208+
if len(respStr) == 0 {
209+
respStr = "(empty)"
210+
}
211+
bodyErr := fmt.Errorf("body: %s", respStr)
208212

209213
switch resp.StatusCode {
210214
case http.StatusTooManyRequests,
211215
http.StatusBadGateway,
212216
http.StatusServiceUnavailable,
213217
http.StatusGatewayTimeout:
214218
// Retryable failure.
215-
216-
var err error
217-
if len(respStr) > 0 {
218-
// include response body for context
219-
err = errors.New(respStr)
220-
}
221-
return newResponseError(resp.Header, err)
219+
return newResponseError(resp.Header, bodyErr)
222220
default:
223221
// Non-retryable failure.
224-
225-
if len(respStr) > 0 {
226-
// include response body for context
227-
e := errors.New(respStr)
228-
return fmt.Errorf("failed to send to %s: %s (%w)", request.URL, resp.Status, e)
229-
}
230-
return fmt.Errorf("failed to send to %s: %s", request.URL, resp.Status)
222+
return fmt.Errorf("failed to send to %s: %s (%w)", request.URL, resp.Status, bodyErr)
231223
}
232224
})
233225
}

0 commit comments

Comments
 (0)