use AddError everywhere #2372
use AddError everywhere #2372
Conversation
| return a.Errors[0] | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
I'm open to feedback on this method. I just wrote it as it made adjusting the tests a lot easier by not having to check both the return value of Gather() and any errors added by AddError().
There was a problem hiding this comment.
I think I would prefer to accept a telegraf.Input interface, and then call the Gather() method on that interface instead of accepting the Gather function signature.
There was a problem hiding this comment.
Agreed, but it's not critical, we can improve this latter
There was a problem hiding this comment.
Another improvement might be to add an acc.AddErrorf(). The majority of the usage is nesting an fmt.Errorf() inside acc.AddError(). Would just be a convenience.
There was a problem hiding this comment.
Long term I think we need to allow access to log level as well, probably using a good logging library that allows control at a per logger basis (and one logger per plugin).
There was a problem hiding this comment.
I proposed this a few times, but was always declined by your predecessor. Not sure what the reasons were though.
#1446 (comment)
#2326 (comment)
| @@ -175,7 +175,7 @@ func TestHttpJsonOn404(t *testing.T) { | |||
|
|
|||
| var acc testutil.Accumulator | |||
| acc.SetDebug(true) | |||
| err := jolokia.Gather(&acc) | |||
| err := acc.GatherError(jolokia.Gather) | |||
|
|
|||
| assert.Nil(t, err) | |||
There was a problem hiding this comment.
This test is failing because of a similar situation to the cassandra one. The code was previously printing an error, but is now being passed to AddError(). The problem here is that this test doesn't make much sense. It generates a stub that returns valid response JSON, but with a http 404 status code. Jolokia will not do this, thus the test seems bogus.
Given that the cassandra code seems to be based off the jolokia code, I'm betting the cassandra test that I previously commented on is bogus too.
| @@ -197,24 +197,25 @@ func TestHttpJsonJavaMultiType(t *testing.T) { | |||
| } | |||
|
|
|||
| // Test that the proper values are ignored or collected | |||
| func TestHttpJsonOn404(t *testing.T) { | |||
| func TestHttp404(t *testing.T) { | |||
There was a problem hiding this comment.
As a resolution to the earlier comment on this test, I'm changing the test to look for the error that is now passed via AddError(). Previous behavior of the plugin was that it was ignoring http-404 errors, and instead printing (not returning) an error when it failed to parse the response body. New behavior calls AddError() on both non-http-200 responses, as well as parse errors.
The cassandra plugin test was modified for the same reason.
|
CircleCI test is failing due to #2382 . Just keep running it. Eventually it'll pass. |
| return a.Errors[0] | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Agreed, but it's not critical, we can improve this latter
Yeah, I doubt it'll be hard. When do you need it by? Hopefully I'll take care of it tonight. |
|
That will be good, probably try to branch for the release sometime early next week. |
|
Rebased. Tests passing. |
Required for all PRs:
Closes #1446
Also addressed some of the issues described in #2326 (comment) by cleaning up some of the
fmt.Printf()andlog.Print()statements to useacc.AddError().