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

Feature/tags for elasticsearchwriter #10074

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

SebastianOpeni
Copy link
Contributor

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

Copy link

cla-bot bot commented Jun 7, 2024

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.

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

@ngoeddel-openi ngoeddel-openi left a comment

Choose a reason for hiding this comment

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

lgtm

@SebastianOpeni
Copy link
Contributor Author

The CLA should be signed now.

@bobapple
Copy link
Member

bobapple commented Jun 7, 2024

@cla-bot check

@cla-bot cla-bot bot added the cla/signed label Jun 7, 2024
Copy link
Member

@yhabteab yhabteab left a comment

Choose a reason for hiding this comment

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

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!

.mailmap Outdated Show resolved Hide resolved
doc/21-development.md Outdated Show resolved Hide resolved
lib/perfdata/elasticsearchwriter.hpp Outdated Show resolved Hide resolved
lib/perfdata/elasticsearchwriter.ti Outdated Show resolved Hide resolved
lib/perfdata/elasticsearchwriter.ti Outdated Show resolved Hide resolved
lib/perfdata/elasticsearchwriter.hpp Outdated Show resolved Hide resolved
lib/perfdata/elasticsearchwriter.cpp Outdated Show resolved Hide resolved
lib/perfdata/elasticsearchwriter.cpp Outdated Show resolved Hide resolved
@yhabteab yhabteab added this to the 2.15.0 milestone Sep 13, 2024
@yhabteab yhabteab added the enhancement New feature or request label Sep 13, 2024
@SebastianOpeni
Copy link
Contributor Author

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

@SebastianOpeni SebastianOpeni force-pushed the feature/tags-for-elasticsearchwriter-6837 branch from 44a55ec to 2f3c2fa Compare September 23, 2024 10:45
@SebastianOpeni
Copy link
Contributor Author

Thanks for your answers in the review.
I tried to implement everything as requested.

lib/perfdata/elasticsearchwriter.cpp Outdated Show resolved Hide resolved
lib/perfdata/elasticsearchwriter.cpp Outdated Show resolved Hide resolved
lib/perfdata/elasticsearchwriter.cpp Outdated Show resolved Hide resolved
.mailmap Outdated Show resolved Hide resolved
doc/21-development.md Outdated Show resolved Hide resolved
@yhabteab
Copy link
Member

Apart from #10074 (comment), code wise it should be fine now, I haven't tested it with a actual ElasticSearch instance though, but till I get the time, some of my colleagues might want a look at this as well.

doc/14-features.md Outdated Show resolved Hide resolved
lib/perfdata/elasticsearchwriter.cpp Outdated Show resolved Hide resolved
Al2Klimov

This comment was marked as outdated.

Copy link
Member

@yhabteab yhabteab left a comment

Choose a reason for hiding this comment

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

I tested this today with ElasticSearch 7.17.25 and it seems to be working fine 👍🏼. Though , whilst going through the changes again, I just noticed two small things, see inline comments below.

Edit: While you're at it, please rebase this against the master branch to get the GHAs pass.

doc/14-features.md Outdated Show resolved Hide resolved
lib/perfdata/elasticsearchwriter.cpp Outdated Show resolved Hide resolved
@SebastianOpeni SebastianOpeni force-pushed the feature/tags-for-elasticsearchwriter-6837 branch from b64788f to 14c47d2 Compare November 26, 2024 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/elastic Events to Elasticsearch cla/signed enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support host and service templates to send variables to Elasticsearch
6 participants