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

[dev.icinga.com #9858] Gelf module: expose 'perfdata' fields for 'CHECK_RESULT' events #3237

Closed
icinga-migration opened this issue Aug 6, 2015 · 13 comments

Comments

Projects
None yet
1 participant
@icinga-migration
Copy link
Member

commented Aug 6, 2015

This issue has been migrated from Redmine: https://dev.icinga.com/issues/9858

Created by daniilyar on 2015-08-06 14:30:18 +00:00

Assignee: mariussturm
Status: Resolved (closed on 2015-12-18 10:15:03 +00:00)
Target Version: 2.5.0
Last Update: 2015-12-18 10:15:03 +00:00 (in Redmine)

Backport?: No
Include in Changelog: 1

Current GELF Icinga plugin output for check results doesn't include any 'perfdata' fields. Instead GELF output includes human-readable, but not highly parseable strings (like 'shortmessage') which contains only a part of meaningful check result. This makes GELF plugin almost impossible to use for graphing and analysing Icinga check results somewhere else outside the Icinga. E.g., at popular Elasticsearch + Logstash + Kibana stack. This is very inconvenient from the point of Logstash (or any other monitoring data parsing tools coming with a Gelf input) as all data parsing tools needs input data to be highly parseable.

Also there is a bunch of Icinga internal variables which, I think, are good to be exposed in GELF plugin CHECK RESULT events, too. E.g., 'checkable->IsReachable()' field could be useful at GELF receiver side for alerting purposes.

I've made a pull request at GitHub which exposes all 'perfdata' fields and 'checkable->IsReachable()' field at GELF plugin 'CHECK RESULT' events: #43
You could get more info there.

Changesets

2015-12-18 10:05:38 +00:00 by (unknown) d739675

GelfWriter: Add additional fields for 'CHECK RESULT' events

fixes #9858

2015-12-18 10:10:54 +00:00 by mfriedrich 5aa4700

Update AUTHORS

refs #9858

2016-02-23 15:28:04 +00:00 by mfriedrich c256ea1

Update AUTHORS

refs #9858
@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2015

Updated by gbeutner on 2015-08-11 06:28:46 +00:00

Stalled until dnsmichi is back.

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2015

Updated by mfriedrich on 2015-08-17 19:01:32 +00:00

  • Status changed from New to Assigned
  • Assigned to set to bernd

Imho that's a question for the Graylog guys. I'm assigning this to Bernd now :)

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2015

Updated by bernd on 2015-08-19 10:04:32 +00:00

Thanks for the heads-up and the pull request. I will talk to my colleague Marius who wrote the initial GELF support and we will review and test the PR.

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Oct 13, 2015

Updated by gbeutner on 2015-10-13 08:04:01 +00:00

Are there any news for this?

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Oct 20, 2015

Updated by mariussturm on 2015-10-20 10:37:15 +00:00

  • Assigned to changed from bernd to mariussturm

Sorry, took me awhile to get my dev account running.
@daniilyar as I see the PR there is no switch to disable additional metrics collection?
Storing raw metrics data in Elasticsearch is pretty expensive and should be configurable. Is this something you could add to the PR?

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Nov 25, 2015

Updated by daniilyar on 2015-11-25 12:21:43 +00:00

Sorry, I didn't receive an email notification about your answer for some weird reason.
Please expect me to provide an update to PR this week. What else would you like me to fix?

mariussturm wrote:

Sorry, took me awhile to get my dev account running.
@daniilyar as I see the PR there is no switch to disable additional metrics collection?
Storing raw metrics data in Elasticsearch is pretty expensive and should be configurable. Is this something you could add to the PR?

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Nov 25, 2015

Updated by mariussturm on 2015-11-25 13:06:01 +00:00

rest is fine, just a minor typo :) daniilyar@208e0ad#diff-db284b572b8d58fd607830d73594f479R143

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Nov 26, 2015

Updated by daniilyar on 2015-11-26 22:12:02 +00:00

rest is fine, just a minor typo :) daniilyar@208e0ad#diff-db284b572b8d58fd607830d73594f479R143

aww, here is why our patched Icinga logs 'invalid perfdata' errors on behalf of OpenTsdbWriter which we don't use ) Fixed.

I've switched adding perfdata fields OFF by default, so folks who already use GelfWriter will only notice 3 new fields: 'latency', 'execution_time' and 'reachable'.
To switch adding perfdata fields ON 'include_perfdata_fields' option should be changed to 'true' at /etc/icinga2/features-available/gelf.conf

Is there any docs for GelfWriter module I can extend with new option?

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Dec 7, 2015

Updated by mariussturm on 2015-12-07 10:57:17 +00:00

Tested this one and looks good to me now!

@dnsmichi could you merge please?

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Dec 7, 2015

Updated by mfriedrich on 2015-12-07 12:29:43 +00:00

Can you provide a feature branch on git.icinga.org with the changes squashed into one single commit for review? Merci.

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Dec 7, 2015

Updated by mariussturm on 2015-12-07 21:46:43 +00:00

@dnsmichi should be now in feature/gelf-perfdata-9858

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2015

Updated by mfriedrich on 2015-12-18 10:09:45 +00:00

  • Target Version set to 2.5.0
  • Backport? changed from Not yet backported to No

Review

  1. Code Style
  • Line-breaks for if-conditions
  • Log() with shift operators require a new line with 4 spaces indent
  • It's not necessary to include the commented config option inside the gelf.conf file, as it is not a mandatory one. Users shall read the documentation, especially getting to know its meaning from the "description" table entry.
  1. Naming of the configuration attribute

I've renamed it to "enable_send_perfdata" to reflect its purpose, in a similar naming pattern as we use in the GraphiteWriter feature.

  1. Documentation

The attribute was missing in the 6-object-types.md table. When sending patches adding or changing things, please always add/update the documentation.

@icinga-migration

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2015

Updated by Anonymous on 2015-12-18 10:15:03 +00:00

  • Status changed from Assigned to Resolved
  • Done % changed from 0 to 100

Applied in changeset d739675.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.