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.

Sign up for GitHub

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

Jump to bottom

Add PowerDNS Recursor input plugin #4545

Merged
merged 2 commits into from
Apr 23, 2019
Merged

Add PowerDNS Recursor input plugin #4545

merged 2 commits into from
Apr 23, 2019

Conversation

dupondje
Copy link
Contributor

Added a PowerDNS Recursor input plugin.

The PowerDNS plugin does not work for the recursor, as it as some differences.
Not only the command to get the stats is different, but also the socket type (datagram socket for the recursor).

As I also had to add some additional options, it looks better to me to have it in a separate plugin.

Please review the code if it can be merged. If things need to get changed, let me know!
I also get a strange error on make test ... so might need some assistance there!

@dupondje dupondje force-pushed the pdns_recursor branch 2 times, most recently from f2f2344 to 56a8244 Compare August 10, 2018 14:59
glinton
glinton suggested changes Aug 13, 2018
View reviewed changes
plugins/inputs/powerdns_recursor/powerdns_recursor.go Outdated

func (p *PowerdnsRecursor) gatherServer(address string, acc telegraf.Accumulator) error {
var randomNumber int64
binary.Read(rand.Reader, binary.LittleEndian, &randomNumber)
Copy link
Contributor

Choose a reason for hiding this comment

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

Overkill and non-performant, consider other options. (atomic counter, etc...)

plugins/inputs/powerdns_recursor/powerdns_recursor_test.go Outdated
func TestPowerdnsRecursorGeneratesMetrics(t *testing.T) {
// We create a fake server to return test data
var randomNumber int64
binary.Read(rand.Reader, binary.LittleEndian, &randomNumber)
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, very overkill for a test, just set randomNumber to a number.

@dupondje
Copy link
Contributor Author

Adjusted the requested changes.

That random came from the powerdns input plugin by the way:
https://github.com/influxdata/telegraf/blob/master/plugins/inputs/powerdns/powerdns_test.go#L76

So might want to change it there also :)

@dupondje dupondje mentioned this pull request Aug 14, 2018
Closed
@dupondje
Copy link
Contributor Author

And if merged, #812 can be closed!

@dupondje dupondje mentioned this pull request Aug 14, 2018
Merged
@glinton
Copy link
Contributor

glinton commented Aug 14, 2018

@dupondje Can you investigate what might be taking so long in the tests? They are failing circleci, I thought it was due to the rand things, but even without them, it seems to be taking too long.
Thanks

@dupondje
Copy link
Contributor Author

Test was fixed! :)

It was caused by the fact that with Unixgram sockets, you need to write the message to the remote socket, and not to the socket the message was read from.
So the Read succeeded, but the Write wasn't received by the plugin, so it was hanging forever ...

@dupondje
Copy link
Contributor Author

@danielnelson: Any changes required to get it merged?

@danielnelson danielnelson self-requested a review August 23, 2018 20:34
@dupondje
Copy link
Contributor Author

dupondje commented Oct 3, 2018

What are we waiting for? :) Would like to get it merged.

@dupondje
Copy link
Contributor Author

Any update for this? Would be nice to get it merged!

danielnelson
danielnelson suggested changes Apr 19, 2019
View reviewed changes
Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

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

Sorry about the delay and thanks for being patient. Looks good but there are a few things still to resolve:

plugins/inputs/powerdns_recursor/powerdns_recursor.go Outdated Show resolved Hide resolved
plugins/inputs/powerdns_recursor/powerdns_recursor.go Outdated Show resolved Hide resolved
plugins/inputs/powerdns_recursor/powerdns_recursor.go Outdated Show resolved Hide resolved
plugins/inputs/powerdns_recursor/powerdns_recursor.go Outdated Show resolved Hide resolved
plugins/inputs/powerdns_recursor/powerdns_recursor.go Outdated Show resolved Hide resolved
plugins/inputs/powerdns_recursor/README.md Outdated Show resolved Hide resolved
plugins/inputs/powerdns_recursor/powerdns_recursor.go Outdated Show resolved Hide resolved
@dupondje dupondje force-pushed the pdns_recursor branch 2 times, most recently from 34a5e03 to c2027cb Compare April 19, 2019 15:22
@glinton
Copy link
Contributor

glinton commented Apr 22, 2019

Can you update your branch with master please?

@dupondje dupondje force-pushed the pdns_recursor branch 2 times, most recently from 5e9b8df to 61fa896 Compare April 22, 2019 22:09
@dupondje
Copy link
Contributor Author

Updated :)

@danielnelson
Copy link
Contributor

@dupondje I think we just need the tests fixed and we should be good to merge.

@dupondje
Copy link
Contributor Author

Should be good now!

@danielnelson danielnelson merged commit a020274 into influxdata:master Apr 23, 2019
@danielnelson danielnelson added this to the 1.11.0 milestone Apr 23, 2019
hwaastad pushed a commit to hwaastad/telegraf that referenced this pull request Jun 13, 2019
@dupondje
Add PowerDNS Recursor input plugin (influxdata#4545)
33f95e1
bitcharmer pushed a commit to bitcharmer/telegraf that referenced this pull request Oct 18, 2019
@dupondje @bitcharmer
Add PowerDNS Recursor input plugin (influxdata#4545)
84ee442
athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020
@dupondje
Add PowerDNS Recursor input plugin (influxdata#4545)
11867a2
idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020
@dupondje
Add PowerDNS Recursor input plugin (influxdata#4545)
fe1012f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielnelson danielnelson danielnelson approved these changes

@glinton glinton glinton approved these changes

Assignees
No one assigned
Labels
new plugin
Projects
None yet
Milestone
1.11.0
Development

Successfully merging this pull request may close these issues.
3 participants
@dupondje @glinton @danielnelson