[exporter/datadog] Do not read response if nil on logs error #16390
Conversation
| if r != nil { | ||
| b := make([]byte, 1024) // 1KB message max | ||
| n, _ := r.Body.Read(b) // ignore any error | ||
| s.logger.Error("Failed to send logs", zap.Error(err), zap.String("msg", string(b[:n])), zap.String("status_code", r.Status)) |
There was a problem hiding this comment.
Do you still want to log the error if r is nil?
There was a problem hiding this comment.
Do you still want to log the error if r is nil?
It will be logged by the exporterhelper since we bubble up the error :)
I think the real question is if we should be double logging when r is not nil, I feel like it's justified because we are adding some extra fields here, and it's hard to pass these as fields to the exporterhelper
|
|
||
| // If response is nil assume permanent error. | ||
| // The error will be logged by the exporter helper. | ||
| return consumererror.NewPermanent(err) |
There was a problem hiding this comment.
If the server is unreachable , i would expect this code to get hit. Shouldn't we retry in that case ?
|
@dineshg13 @bogdandrutu : is it intentional behavior that any other error other than specific http codes from the datadog server are considered permanent? If there are temporary network blips that cause connectivity issues to datadog, that wouldn't result in a http error code, it would result in a connect error - but those should be retried in case network comes back up later. Could you please clarify your thinking about intermittent network blips / temporary network partition? |
Description:
Do not try to read the response body if nil. The
SubmitLogsmethod returns an error when failing to submit (e.g. 4xx or 5xx) but also in other cases where there is no response. The latter are classified as permanent errors to avoid unnecessary retries.Link to tracking Issue: Fixes #16077