Skip to content

notifications: Attribute Negotiation#1120

Open
oxzi wants to merge 1 commit into
mainfrom
notifications-attribute-negotiation
Open

notifications: Attribute Negotiation#1120
oxzi wants to merge 1 commit into
mainfrom
notifications-attribute-negotiation

Conversation

@oxzi
Copy link
Copy Markdown
Member

@oxzi oxzi commented May 22, 2026

Initial support for attribute negotiation, the big change with Icinga Notifications v1.0.

Now, the notifications.Client does not evaluate the rules by itself anymore, but passes a subset of available information to Icinga Notifications. From there on, Icinga Notifications might return multiple JSONPath objects, which are being used to enrich the submitted event.

This change required an adjustment to the icinga-go-library, which was bumped here to its PR branch.


The IGL change was suggested in Icinga/icinga-go-library#201.

Fixes #1098.

@oxzi oxzi added this to the 1.6.0 milestone May 22, 2026
@oxzi oxzi requested a review from yhabteab May 22, 2026 12:35
@oxzi oxzi added the area/notifications Icinga Notifications Integration label May 22, 2026
@cla-bot cla-bot Bot added the cla/signed label May 22, 2026
@oxzi oxzi force-pushed the notifications-attribute-negotiation branch from de14b08 to 3f347ee Compare May 26, 2026 12:39
@oxzi
Copy link
Copy Markdown
Member Author

oxzi commented May 26, 2026

Updated the PR after talking about it with @yhabteab offline. Next to some smaller bugfixes and changes, the following more prominent changes were made:

  • Bulk fetch Redis objects of the same type via HMGET.
  • Bulk fetch custom vars from SQL via IN queries.
  • Track complete relations and submit with the event.
  • Drop fetching custom vars for host and service groups.

@oxzi oxzi force-pushed the notifications-attribute-negotiation branch from 3f347ee to 208bf74 Compare May 26, 2026 13:20
@oxzi
Copy link
Copy Markdown
Member Author

oxzi commented May 26, 2026

Sorry, pushed a small change to ensure that the complete relations are always populated and remove duplicate code.

Copy link
Copy Markdown
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.

Quote from #1098:

If Icinga DB is configured to always include particular relations, it can signal Icinga Notifications that these are complete and cannot be updated by using a request header: X-Icinga-Complete-Relations: host.vars, services[*].vars

This is still missing!

Comment thread pkg/notifications/fetch.go Outdated
Comment thread pkg/notifications/fetch.go Outdated
Comment thread pkg/notifications/fetch.go Outdated
Comment thread pkg/notifications/fetch.go Outdated
Comment thread pkg/notifications/fetch.go Outdated
Comment thread pkg/notifications/notifications.go Outdated
@oxzi oxzi force-pushed the notifications-attribute-negotiation branch from 208bf74 to d97709c Compare May 27, 2026 16:10
@oxzi
Copy link
Copy Markdown
Member Author

oxzi commented May 27, 2026

Updated the PR and tried to address all of @yhabteab's review comments. Most prominent, the following changes were made:

  • Limit object fields to name and display_name.
  • Only fetch host and service object information by default, no custom vars or groups anymore.
  • Inspect the provided JSONPath in more detail and re-fetch required attributes.
  • Add a minimal test suite.

However, I still have to address the main review comment, which I am going to do tomorrow. Afterwards, maybe a bit of refactoring would be pleasant for the reviewer's eyes.

@oxzi
Copy link
Copy Markdown
Member Author

oxzi commented May 28, 2026

After a bit of internal refactoring and adding configuration options, I would kindly ask you to take another look, @yhabteab. All prior comments should have been addressed in the diff.

@oxzi oxzi requested a review from yhabteab May 28, 2026 14:47
Copy link
Copy Markdown
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've left few comments from briefly looking at the code. But, I can tell you that it looks way better than the previous version. I need to spend some more time to understand the relationsProvider implementation though, since it seems to be doing a lot of things. I'll continue to review the code tomorrow!

Comment thread internal/config/config.go Outdated
Comment thread pkg/notifications/fetch.go
Comment thread pkg/notifications/fetch.go Outdated
Comment thread pkg/notifications/fetch.go Outdated
Comment thread pkg/notifications/fetch.go
Comment thread pkg/notifications/fetch.go Outdated
Comment thread pkg/notifications/fetch.go Outdated
Comment thread pkg/notifications/fetch.go
Comment thread pkg/notifications/fetch.go Outdated
Comment thread pkg/notifications/notifications.go Outdated
Comment thread pkg/notifications/notifications.go Outdated
Comment thread pkg/notifications/fetch.go Outdated
@oxzi oxzi force-pushed the notifications-attribute-negotiation branch from 2e2c03d to 2493aff Compare May 29, 2026 13:57
@oxzi
Copy link
Copy Markdown
Member Author

oxzi commented May 29, 2026

Tried to address all comments next to some changes, such as a combined SQL query to fetch host and service groups instead of an SQL query for IDs and a Redis query afterwards.

@oxzi oxzi requested a review from yhabteab May 29, 2026 13:58
@oxzi oxzi force-pushed the notifications-attribute-negotiation branch 2 times, most recently from e4c907a to 8caa1e1 Compare May 29, 2026 14:58
Initial support for attribute negotiation, the big change with Icinga
Notifications v1.0.

Now, the notifications.Client does not evaluate the rules by itself
anymore, but passes a subset of available information to Icinga
Notifications. From there on, Icinga Notifications might return multiple
JSONPath objects, which are being used to enrich the submitted event.

This change required an adjustment to the icinga-go-library, which was
bumped here to its PR branch.
@oxzi oxzi force-pushed the notifications-attribute-negotiation branch from 8caa1e1 to 26569ce Compare May 29, 2026 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/notifications Icinga Notifications Integration cla/signed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for Icinga Notifications v1.0 – Attribute Negotiation

3 participants