Feature/tags for elasticsearchwriter #10074

SebastianOpeni · 2024-06-07T09:41:55Z

Offers to add additional tags to entries written by the ElasticSearchWriter.

As discussed with Eric at the icinga summit we are submitting this feature. Please revise the code and let us know if there is something we need to improve. otherwise we would be happy for timely merge.

fixes #6837

cla-bot · 2024-06-07T09:41:58Z

Thank you for your pull request. Before we can look at it, you'll need to sign a Contributor License Agreement (CLA).

Please follow instructions at https://icinga.com/company/contributor-agreement to sign the CLA.

After that, please reply here with a comment and we'll verify.

Contributors that have not signed yet: @SebastianOpeni

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Please contact us if you think this is the case.
If you signed the CLA as a corporation, your GitHub username may not have been submitted to us. Please reach out to the responsible person in your organization.

ngoeddel-openi

lgtm

SebastianOpeni · 2024-06-07T10:26:40Z

The CLA should be signed now.

bobapple · 2024-06-07T10:34:16Z

@cla-bot check

yhabteab

Hi, thanks for your contribution! I've just been looking over the code changes and left some inline comments below, but haven't tested it with an Elastic/OpenSearch instance yet. Other than that, can you please rebase this against the current master to fix the GitHub actions. Thanks!

yhabteab · 2024-09-13T08:52:39Z

.mailmap

@@ -63,3 +63,4 @@ Robin O'Brien <[email protected]> <[email protected]>
 Roman Gerhardt <[email protected]> <[email protected]>
 Sebastian Chrostek <[email protected]> <[email protected]>
 Thomas Gelf <[email protected]> <[email protected]>
+Sebastian Grund <[email protected]> <[email protected]>


That change is not necessary as both of your mails are the same! .mailmap is actually there to map different mails of an author.

yhabteab · 2024-09-13T09:05:48Z

doc/21-development.md

Can you please commit all the changes in this file separately.

yhabteab · 2024-09-13T09:10:13Z

lib/perfdata/elasticsearchwriter.hpp

+	void ValidateHostTagsTemplate(const Lazy<Dictionary::Ptr> &lvalue, const ValidationUtils &utils);
+	void ValidateServiceTagsTemplate(const Lazy<Dictionary::Ptr> &lvalue, const ValidationUtils &utils);


You are overriding virtual methods here, so please add the override keyword at the end.

yhabteab · 2024-09-13T09:23:54Z

lib/perfdata/elasticsearchwriter.ti

+	[config, required] Dictionary::Ptr host_tags_template {
+		default {{{
+			return new Dictionary({
+			});
+		}}}
+	};
+	[config, required] Dictionary::Ptr service_tags_template {
+		default {{{
+			return new Dictionary({
+			});
+		}}}
+	};


It is unclear to me what the advantage is of labelling these attributes as required and returning an empty dictionary by default. Would it not be more appropriate to omit the required keyword altogether with the default values and simply declare them as [config] Dictionary::Ptr host_tags_template;?

Btw, while these are labelled as required, you've documented them as being optional.

You are absolutely right about the required keyword and I will remove it. 🙈
(To be perfectly honest I took heavy inspiration for this part from the definition of the influx perfdata writer. So most likely it has to be removed there too.)

(To be perfectly honest I took heavy inspiration for this part from the definition of the influx perfdata writer. So most likely it has to be removed there too.)

The InfluxDB Writer doesn't return an empty dictionary by default, instead it requires that the two dictionary keys measurement and tags to be always there, i.e. the attribute host_template is never allowed to be empty. Here on the other hand, there are no really meaningful default values, so it should not be marked as required and the empty default value should be removed as well.

yhabteab · 2024-09-13T09:38:03Z

lib/perfdata/elasticsearchwriter.ti

+	[config, required] Dictionary::Ptr host_tags_template {
+		default {{{
+			return new Dictionary({
+			});
+		}}}
+	};
+	[config, required] Dictionary::Ptr service_tags_template {
+		default {{{
+			return new Dictionary({
+			});
+		}}}
+	};


Are nested dictionaries/arrays supported or only simple key-value pairs? If not, it would be advisable to add such a validator for these two new attributes, otherwise, Icinga 2 will simply fail without any indication of that misconfigured type, e.g. when using such a config: host_tags_template = { os_name = {"foo" = "host.vars.os$"} }

[2024-09-13 11:39:38 +0200] information/ConfigItem: Committing config item(s). [2024-09-13 11:39:38 +0200] critical/config: Error: Operator + cannot be applied to values of type 'String' and 'Dictionary' [2024-09-13 11:39:38 +0200] critical/config: 1 error [2024-09-13 11:39:38 +0200] critical/cli: Config validation failed. Re-run with 'icinga2 daemon -C' after fixing the config.

In my testing netsted dictionarys worked just fine when I remember correctly, but I'm not sure about arrays so I will test both cases.

If it is indeed supported, then you probably need to adjust validation errors, i.e. notice that in my example config snippet the opening $ is missing and when trying to produce an error message, you use the + operator to concatenate string with the nested dictionary ("Closing $ not found in macro format string '" + pair.second and pair.second is the nested dictionary in this case).

My tests had the following results:

1st level nested dictionaries are supported
service_tags_template= { a = { b = "c" } } => tags.a.b = "c"

2nd-level nested dictionaries are valid, but interpreted as (multi line) strings contained in the first nested dictionary.
service_tags_template= { a = { b = { c = "d" } } } => tags.a.b = "{ c= "d" }"

arrays are also valid
service_tags_template= { a = ["b", "c"] } => tags.a = ["b", "c"]

All of them are possible to contain macros therefore I would need a Macro Validator similar to MacroProcessor::ValidateCustomVars.

Would you prefer me:

rewriting a similar method in the elastic-search-writer

or creating a more general method MacroProcessor::ValidateMacroObject (or similar name) that gets called by my validator methods and also by MacroProcessor::ValidateCustomVars?

I would prefer the second option, but it would mean I change the MacroProcessor Class so I wanted to check back with you first.

I would rather question the support for endlessly nested dicts in general before you do such a big change. As far as I'm concerned, I would just accept simple key => value pairs, but I don't know all the use cases you're going to need that for. So my question to you: is it worth making that lot of major code changes and probably investing even more time for testing just to support nested dicts/arrays?

I've looked at some other writers, and none of them support nested dictionaries up to unknown levels. So, if you absolutely need nested tags, couldn't you use such "tags.a.b.c.d" = "$foo$" as simple key-value pairs? This would really simplify the whole thing.

yhabteab · 2024-09-13T09:55:24Z

lib/perfdata/elasticsearchwriter.hpp

@@ -37,6 +40,7 @@ class ElasticsearchWriter final : public ObjectImpl<ElasticsearchWriter>
 	std::mutex m_DataBufferMutex;

 	void AddCheckResult(const Dictionary::Ptr& fields, const Checkable::Ptr& checkable, const CheckResult::Ptr& cr);
+	void AddTemplateTags(const Dictionary::Ptr& fields, const Host::Ptr& host, const Service::Ptr& service, const CheckResult::Ptr& cr);


You can also merge host and service parameters by using Checkable::Ptr instead, just like the other methods in here.

That is true and I also thought about doing so.
My reasoning was the following:
For the Resolver inside of the method I need to get the Host::Ptr and Service::Ptr from the Checkable::Ptr like it is done in other places.
But at all sources where AddTemplateTags will be called from this Host and Service retrieval has been done already just a couple of lines above.
So I would do the same "conversion" multiple times and therefore opted to call the Method with Host::Ptr and Service::Ptr directly.
If you would prefer the Checkable::Ptr way I can also implement it that way.

If you would prefer the Checkable::Ptr way I can also implement it that way.

The thing is, currently it looks like as the service parameter is always required and implies that it may never be a nullptr, thus I would either use Checkable::ptr instead and do the one-line casting again (my preferred variant) or make the service parameter optional.

yhabteab · 2024-09-13T09:58:38Z

lib/perfdata/elasticsearchwriter.cpp

+		ObjectLock olock(tags);
+		for (const Dictionary::Pair& pair : tags) {
+			if (!MacroProcessor::ValidateMacroString(pair.second))
+				BOOST_THROW_EXCEPTION(ValidationError(this, { "host_tags_template", pair.first }, "Closing $ not found in macro format string '" + pair.second));


Missing + "'." after pair.second (this applies to the service tags validation down below as well).

yhabteab · 2024-09-13T10:01:32Z

lib/perfdata/elasticsearchwriter.cpp

@@ -131,6 +132,36 @@ void ElasticsearchWriter::Pause()
 	ObjectImpl<ElasticsearchWriter>::Pause();
 }

+void ElasticsearchWriter::AddTemplateTags(const Dictionary::Ptr& fields, const Host::Ptr& host, const Service::Ptr& service, const CheckResult::Ptr& cr)
+{
+	MacroProcessor::ResolverList resolvers;


You can declare this above or below the ObjectLock olock ... within the if block instead.

SebastianOpeni · 2024-09-18T15:06:30Z

Hi thanks for your review :)
I am currently working on your remarks but might have some questions beforehand.

SebastianOpeni added 3 commits June 7, 2024 11:32

Add tags fuctionality to ElasticSearchWriter

c523d23

Documentation for tags fuctionality in ElasticSearchWriter

90ed892

Update AUTHORS

44a55ec

icinga-probot bot added the area/elastic Events to Elasticsearch label Jun 7, 2024

ngoeddel-openi approved these changes Jun 7, 2024

View reviewed changes

cla-bot bot added the cla/signed label Jun 7, 2024

yhabteab requested changes Sep 13, 2024

View reviewed changes

yhabteab added this to the 2.15.0 milestone Sep 13, 2024

yhabteab added the enhancement New feature or request label Sep 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/tags for elasticsearchwriter #10074

Feature/tags for elasticsearchwriter #10074

SebastianOpeni commented Jun 7, 2024

cla-bot bot commented Jun 7, 2024

ngoeddel-openi left a comment

SebastianOpeni commented Jun 7, 2024

bobapple commented Jun 7, 2024

yhabteab left a comment

yhabteab Sep 13, 2024

yhabteab Sep 13, 2024

yhabteab Sep 13, 2024

yhabteab Sep 13, 2024

SebastianOpeni Sep 18, 2024

yhabteab Sep 19, 2024

yhabteab Sep 13, 2024

SebastianOpeni Sep 18, 2024

yhabteab Sep 19, 2024

SebastianOpeni Sep 19, 2024 •

edited

Loading

yhabteab Sep 19, 2024

yhabteab Sep 13, 2024

SebastianOpeni Sep 18, 2024

yhabteab Sep 19, 2024

yhabteab Sep 13, 2024

yhabteab Sep 13, 2024

SebastianOpeni commented Sep 18, 2024

		void ValidateHostTagsTemplate(const Lazy<Dictionary::Ptr> &lvalue, const ValidationUtils &utils);
		void ValidateServiceTagsTemplate(const Lazy<Dictionary::Ptr> &lvalue, const ValidationUtils &utils);

Feature/tags for elasticsearchwriter #10074

Are you sure you want to change the base?

Feature/tags for elasticsearchwriter #10074

Conversation

SebastianOpeni commented Jun 7, 2024

cla-bot bot commented Jun 7, 2024

ngoeddel-openi left a comment

Choose a reason for hiding this comment

SebastianOpeni commented Jun 7, 2024

bobapple commented Jun 7, 2024

yhabteab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianOpeni Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianOpeni commented Sep 18, 2024

SebastianOpeni Sep 19, 2024 •

edited

Loading