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 unbound input plugin #3434

Merged
merged 10 commits into from
Nov 20, 2017
Merged

Add unbound input plugin #3434

merged 10 commits into from
Nov 20, 2017

Conversation

aromeyer
Copy link
Contributor

@aromeyer aromeyer commented Nov 6, 2017

This plugin gathers stats from Unbound - a validating, recursive, and caching DNS resolver (https://www.unbound.net/)

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

@@ -0,0 +1,3 @@
// +build windows
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's leave this file out, if somehow you have unbound-control on Windows then have at it.

### Tags:

As indicated above, the prefix of a unbound stat will be used as it's 'section' tag. So section tag may have one of
the following values:
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like each section is very different, maybe we should call the measurement unbound_<section> and remove the tag. If we did this we would probably want to do something special for the threadX section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As there is not too much data (no really need to spread information in several measurements) and I do not control unbound-control future evolution, I will start to simply drop section/tag and push all fields as they are (just converting field name dots to underscores).
I will just have a look on the "histograms" fields and see if I drop them or do something intelligible. I am not sure that these fields are really interesting for telegraf as we are collecting timeseries data... we don't really need partially pre-processed historical information.

https://www.unbound.net/documentation/unbound-control.html for details.

- unbound
thread0.num.queries
Copy link
Contributor

Choose a reason for hiding this comment

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

In this section list the fields how they will be emitted from Telegraf, so thread0.num.queries will be num.queries.

https://www.unbound.net/documentation/unbound-control.html for details.

- unbound
thread0.num.queries
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should consider replace dots with underscores the field num_queries so the fields fit style wise with the other plugins.

mem.cache.message
mem.mod.iterator
mem.mod.validator
histogram.000000.000000.to.000000.000001
Copy link
Contributor

Choose a reason for hiding this comment

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

How are these histogram metrics encoded? Is the field named 000000.000000.to.000000.000001? Maybe we can do special processing of these, normally we use the format produced by the histogram aggregator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes actually the field name will be 000000.000000.to.000000.000001 which is not really usefful. Anyway I don't think that this kind of information is relevant for telegraf. It is easier to construct the histogram from telegraf collected data rather than using these fields. I think I will skip them from collected metrics.

if len(s.Stats) == 0 {
s.filter, err = filter.Compile(defaultStats)
} else {
// legacy support, change "all" -> "*":
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't have legacy support already since we are a new plugin :)

// Gather collects the configured stats from unbound_stat and adds them to the
// Accumulator
//
// The prefix of each stat (eg MAIN, MEMPOOL, LCK, etc) will be used as a
Copy link
Contributor

Choose a reason for hiding this comment

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

These aren't actual sections, and there is no unbound_stat. I guess this is holdovers from an earlier version of the code, can you update it?

cols := strings.Split(scanner.Text(), "=")

stat := cols[0]
value := cols[1]
Copy link
Contributor

Choose a reason for hiding this comment

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

This could panic if somehow there are not two fields, make sure to guard against this.

sectionMap[section] = make(map[string]interface{})
}

sectionMap[section][field], err = strconv.ParseUint(value, 10, 64)
Copy link
Contributor

Choose a reason for hiding this comment

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

Parse as int64, since this is what the Accumulator holds, if there is an error parsing don't add to the fields.


sectionMap[section][field], err = strconv.ParseUint(value, 10, 64)
if err != nil {
sectionMap[section][field], err = strconv.ParseFloat(value, 64)
Copy link
Contributor

Choose a reason for hiding this comment

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

This fallback method could switch the type of the field, which will cause the metrics to be impossible to add to InfluxDB. We may need to parse all fields as floats, or have a list of the types.

Alain ROMEYER added 3 commits November 8, 2017 12:37
Timeout is less agresssive and configurable, tags have been removed, field name contains underscores instead of dots, field value are float64...
type Unbound struct {
Stats []string
Binary string
Timeout int
Copy link
Contributor

Choose a reason for hiding this comment

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

Use internal.Duration for this, it will look like timeout = "1s" in toml.

@danielnelson danielnelson added this to the 1.5.0 milestone Nov 9, 2017
As suggested, Timeout is now an internal.Duration object. Readme and telegraf plugin configurations have been updated accordingly. Thanks for your comments.
@@ -0,0 +1,161 @@
// +build !windows
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this build flag, and also in the test file.

## Glob matching can be used, ie, stats = ["total.*"]
## stats may also be set to ["*"], which will collect all stats
## except histogram.* statistics that will never be collected.
stats = ["total.*", "num.*","time.up", "mem.*"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should remove this filter, users can simply use the builtin fielddrop/fieldpass filters to get the same behavior.

* remove useless plugin option
* adapt tests
* update README and telegraf.conf
@danielnelson danielnelson merged commit e544d74 into influxdata:master Nov 20, 2017
maxunt pushed a commit that referenced this pull request Jun 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants