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

OpenTsdbWriter - Custom Tag Support #7357

Merged
merged 7 commits into from Nov 15, 2019

Conversation

Ant1x
Copy link
Contributor

@Ant1x Ant1x commented Jul 24, 2019

Currently the OpenTsdbWriter does not allow any configuration of the tags that are sent with perfdata, it is currently hard-coded to send the hostname, object type and service name as tags.

This PR adds support to define tags to send to OpenTSDB within the config file. These tags are sent in addition to the default ones.

This functionality is almost identical to how InfluxDbWriter allows setting custom tags, which can be user defined custom attributes (vars dictionary). Most of the implementation has been adapted from InfluxDbWriter.

For existing users who are using the OpenTsdbWriter, no functionality is changed unless the config file contains the new (optional) blocks. I didn't want to change too much of the core functionality as it might break upgrades for some users.

This is my first PR; I am happy to work with you on whatever needs to be done.

@Ant1x
Copy link
Contributor Author

Ant1x commented Jul 26, 2019

I have decided I am going to try and re-work this slightly to remove the unnecessary Clone, as per commentary in #7353
I'm also going to introduce some more optimisations such as fetching the config only once per instantiation as opposed to once per perfdata send

@Ant1x
Copy link
Contributor Author

Ant1x commented Jul 26, 2019

Cloning behaviour has been removed, this should now be a bit more efficient.

I'm not too sure if there could be an opportunity for a memory leak in calling the new
OpenTsdbWriter::ReadConfigTemplate()

This will only be called during a resume (startup, reload signal or HA cluster change), but I wanted to make sure that the underlying OpenTsdbWriter::GetServiceTemplate() and OpenTsdbWriter::GetHostTemplate() (these are auto-generated getter functions from the OpenTsdbWriter object's config) will handle destruction of the object they return? If relevant of course, my main concern is what happens during a reload signal or HA change?

Apart from that, I think this is ready for review or further comments.

@dnsmichi
Copy link
Contributor

dnsmichi commented Aug 8, 2019

Thanks. You're not forgotten, I am just too busy. Will review asap.

@dnsmichi dnsmichi self-requested a review August 8, 2019 16:25
@dnsmichi dnsmichi added area/opentsdb Metrics to OpenTSDB enhancement New feature or request labels Aug 8, 2019
@Ant1x
Copy link
Contributor Author

Ant1x commented Oct 23, 2019

I've also added some extra functionality off the back of this PR which provides some more flexibility around choosing OpenTSDB metric names and exposing this in the config file.
I found that quite a bit was hard coded in which can make customising to fit specific needs difficult.
Updated documentation has also been included.

This code added in the latest commit today has been running in production for the last two months on version 2.10.5, in an environment that has 500 hosts and growing, 15 satellites and growing and two masters in HA. The OpenTSDBWriter has been ticking away well during this time.

Further testing has been performed to ensure an older style configuration file behaves in the exact same way as it did before this PR.

This is the last of any feature additions I will add in this PR. It is now ready for review.

Summary of additions:

Metric Prefixes
It is now possible to customise the OpenTSDB metric name in the form of modifying the prefix. This also supports the use of Icinga macros to add flexibility.

Generic Metrics
A hard-coded function exists in the current release where the perfdata label is appended to each OpenTSDB metric name. This can be very problematic for some checks which use dynamic names for perfdata labels such as Windows network checks where each perfdata label is made up of the hardware adapter name displayed in Windows.
The result is an unmanageable list of OpenTSDB metric names and aggregation of data across different servers with different hardware is cumbersome.

Example:

icinga.service.network-windows.HP-Ethernet-10Gb-2-port-530-FLR-SFP
icinga.service.network-windows.Broadcom-NetXtreme-Gigabit-Ethernet
icinga.service.network-windows.Network-2

This can still be helpful in some situations, and the default functionality has not been modified at all to ensure current users are not impacted when they upgrade.

Generic metrics removes the perfdata label from the metric name and instead adds it as a label tag to each time series data point. This way, all data can be saved in a single OpenTSDB metric and data can be aggregated and queried easily.

icinga.service.network-windows

@dnsmichi dnsmichi added this to the 2.13.0 milestone Nov 2, 2019
@dnsmichi dnsmichi modified the milestones: 2.13.0, 2.12.0 Nov 15, 2019
@dnsmichi
Copy link
Contributor

Awesome, thanks for your hard work on this 👍 Also, the documentation is top notch :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/opentsdb Metrics to OpenTSDB enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants