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 Icinga2 input plugin #2668 #4559

Merged
merged 26 commits into from
Aug 23, 2018
Merged

Conversation

mlabouardy
Copy link
Contributor

@mlabouardy mlabouardy commented Aug 15, 2018

Resolves #2658

Required for all PRs:

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

@mlabouardy mlabouardy mentioned this pull request Aug 15, 2018
3 tasks
Copy link
Contributor

@glinton glinton left a comment

Choose a reason for hiding this comment

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

Thanks for this!

# tls_cert = "/etc/telegraf/cert.pem"
# tls_key = "/etc/telegraf/key.pem"
## Use TLS but skip chain & host verification
`
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing insecure_skip_verify example line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added

# tls_cert = "/etc/telegraf/cert.pem"
# tls_key = "/etc/telegraf/key.pem"
## Use TLS but skip chain & host verification
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Add # insecure_skip_verify = false

# server = "https://localhost:5665"

## Required Icinga2 object type ("services" or "hosts, default "services")
# filter = "services"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe make this a bool, something like defaulting to hosts=false, meaning it will collect services only; unless there are other object types besides services and hosts

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually prefer this style, if we have hosts=false then it isn't clear that it means services. Also if another object type has support added then a bool won't scale up to additional items. I also wonder if a user would ever want to collect both services and hosts? Maybe this should be an array: ["services", "hosts"]? Finally, filter is a somewhat confusing because normally you filter things out, maybe it should be renamed object_type?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh ya, good call

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, i renamed to object_type

return err
}

req.SetBasicAuth(i.Username, i.Password)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe only set this if Username/Password are not empty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@mlabouardy
Copy link
Contributor Author

@glinton done

@mlabouardy
Copy link
Contributor Author

when this will be merged


### Measurements & Fields:

- ll measurements have the following fields:
Copy link
Contributor

Choose a reason for hiding this comment

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

Small typo :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thanks :)

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.

I think there are just a few tweaks needed:

README.md Outdated
@@ -165,6 +165,8 @@ configuration options.
* [haproxy](./plugins/inputs/haproxy)
* [hddtemp](./plugins/inputs/hddtemp)
* [httpjson](./plugins/inputs/httpjson) (generic JSON-emitting http service plugin)
* [icinga2](./plugins/inputs/icinga2)
* [internal](./plugins/inputs/internal)
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 fix the sorting of the plugin, internal is already listed so you can remove that line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

- All measurements have the following tags:
- check_command
- display_name
- source
Copy link
Contributor

Choose a reason for hiding this comment

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

Although not documented yet, as discussed in #4413 we are planning to standardize on source as the hostname of the metric source and split out other components. Can you change the style of these tags to be:

source=example.org,port=5665,scheme=https

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


- All measurements have the following fields:
- name (string)
- state (int)
Copy link
Contributor

Choose a reason for hiding this comment

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

We recently established a pattern for encoding enum like values in several plugins, the basic idea is to use a tag with a string description, and a field with the code:

icinga2,state=warning state_code=1

It looks like we would need a mapping function for each object type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

"github.com/influxdata/telegraf/testutil"
)

func TestGatherStatus(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a unit test for object_type = "hosts" as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1,62 @@
# Example Input Plugin
Copy link
Contributor

Choose a reason for hiding this comment

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

Icinga2 Input Plugin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

tags["check_command"] = check.Attrs.CheckCommand
tags["source"] = i.Server

acc.AddFields(fmt.Sprintf("icinga2_%s_status", i.ObjectType), fields, tags)
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: consider dropping the _status suffix here, it feels superfluous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@mlabouardy
Copy link
Contributor Author

@danielnelson @glinton I have implemented all the required changes :)

@danielnelson danielnelson added this to the 1.8.0 milestone Aug 23, 2018
@danielnelson danielnelson merged commit e72fab7 into influxdata:master Aug 23, 2018
rgitzel pushed a commit to rgitzel/telegraf that referenced this pull request Oct 17, 2018
otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
otherpirate pushed a commit to otherpirate/telegraf that referenced this pull request Mar 15, 2019
dupondje pushed a commit to dupondje/telegraf that referenced this pull request Apr 22, 2019
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.

4 participants