-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Enhancement: Add path to Splunk errors #39026
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
Enhancement: Add path to Splunk errors #39026
Conversation
} else { | ||
err = multierr.Append(err, err2) | ||
} | ||
err = consumererror.NewPermanent(err) |
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.
I also think this change deserves its own PR, given that this code is shared with the splunkhec exporter.
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.
I had to keep the change in so that on permanent errors it is also returned.
I also have some concerns around returning the dumped HTTP response.
I think this change is worth an enhancement changelog. I would caution keeping it just on the addition of the url to the error, and make other improvements separately. |
f4f3680
to
6dca0a2
Compare
887ed33
to
f34a22f
Compare
@@ -1316,7 +1317,12 @@ func TestErrorReceived(t *testing.T) { | |||
case <-time.After(5 * time.Second): | |||
t.Fatal("Should have received request") | |||
} | |||
assert.EqualError(t, err, "HTTP 500 \"Internal Server Error\"") | |||
errMsg := fmt.Sprintf("HTTP %q %d %q", | |||
cfg.ClientConfig.Endpoint, |
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.
The Splunk HEC endpoint can contain auth parameters.
ddb7187
to
34f695f
Compare
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.
LGTM
#### Description To help clarify what endpoint is being used within the collector once errors are being reported, this makes it apparent to all which is the impacted endpoint. #### Link to tracking issue Tracked externally #### Testing Updated existing tests. #### Documentation No user facing changes made, no additional documentation required.
#### Description To help clarify what endpoint is being used within the collector once errors are being reported, this makes it apparent to all which is the impacted endpoint. #### Link to tracking issue Tracked externally #### Testing Updated existing tests. #### Documentation No user facing changes made, no additional documentation required.
#### Description To help clarify what endpoint is being used within the collector once errors are being reported, this makes it apparent to all which is the impacted endpoint. #### Link to tracking issue Tracked externally #### Testing Updated existing tests. #### Documentation No user facing changes made, no additional documentation required.
Description
To help clarify what endpoint is being used within the collector once errors are being reported, this makes it apparent to all which is the impacted endpoint.
Link to tracking issue
Tracked externally
Testing
Updated existing tests.
Documentation
No user facing changes made, no additional documentation required.