Skip to content
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 AddError method to accumulator #1536

Merged
merged 1 commit into from
Jul 25, 2016
Merged

add AddError method to accumulator #1536

merged 1 commit into from
Jul 25, 2016

Conversation

phemmer
Copy link
Contributor

@phemmer phemmer commented Jul 22, 2016

This adds an AddError() method to the accumulator. Ref #1446

As opposed to the original POC in #1446, I instead just implemented it so that AddError() immediately writes out to the log instead of sending it into a chan just so that a goroutine could then write it out to the log. This approach is much simpler.

I also added an errCount to the accumulator which will increment each time an error is generated. This was done for 2 reasons:

  1. So that the Test() function could tell if any errors were generated during the run.
  2. To pave the way for Telegraf should send stats about itself #1348.
    It would be very useful to know when a plugin starts throwing errors. You could then write a kapacitor alert to trigger when such starts happening.

Haven't updated CHANGELOG since the code in this PR is unused, and thus there is no user visible change.

return
}
atomic.AddUint64(&ac.errCount, 1)
log.Printf("ERROR in input [%s]: %s", ac.inputConfig.Name, err)
Copy link
Contributor

@sparrc sparrc Jul 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reading ac.inputConfig.Name could produce a race condition here. Since this bit of code shouldn't be hugely high-traffic, I would remove the atomic package, add a sync.Mutex to the accumulator, and change to:

    if err == nil {
        return
    }
    ac.Lock()
    ac.errCount++
    log.Printf("ERROR in input [%s]: %s", ac.inputConfig.Name, err)
    ac.Unlock()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only place where inputConfig.Name is touched is in internal/config/config.go:798 (during initialization). It is never read or written to ever again (prior to this PR it was an unused variable). Thus there's no need to protect it.

Copy link
Contributor

@sparrc sparrc Jul 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes but this function is reading it, so unless you make the function a value receiver, there can be a race condition here. nevermind, not sure why I was under the impression that reading struct fields was not thread-safe, should be fine...

@phemmer
Copy link
Contributor Author

phemmer commented Jul 22, 2016

How do you want to handle all the failures due to test mocks not having the AddError() method (not satisfying the interface)?
Comment the line out of the interface until the next PR which starts converting the plugins? or do the conversion in this PR?

@sparrc
Copy link
Contributor

sparrc commented Jul 22, 2016

You shouldn't have to update a bunch of test mocks, just add the AddError function to the testutil struct: https://github.com/influxdata/telegraf/blob/master/testutil/accumulator.go#L27

@phemmer
Copy link
Contributor Author

phemmer commented Jul 22, 2016

Ah, didn't look into the issue too deeply (at all really). Will do that then :-)

@phemmer
Copy link
Contributor Author

phemmer commented Jul 24, 2016

Updated PR with the testutil changes.

@sparrc
Copy link
Contributor

sparrc commented Jul 25, 2016

looks great, thanks @phemmer

@sparrc sparrc closed this Jul 25, 2016
@sparrc sparrc reopened this Jul 25, 2016
@sparrc sparrc merged commit e68f251 into influxdata:master Jul 25, 2016
aurrelhebert pushed a commit to aurrelhebert/telegraf that referenced this pull request Aug 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants