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

Doc: Troubleshooting: Optimise apply rules and group assign conditions #9585

Merged
merged 1 commit into from Feb 3, 2023

Conversation

Al2Klimov
Copy link
Member

@Al2Klimov Al2Klimov added enhancement New feature or request area/configuration DSL, parser, compiler, error handling area/documentation End-user or developer help labels Nov 16, 2022
@Al2Klimov Al2Klimov added this to the 2.14.0 milestone Nov 16, 2022
@cla-bot cla-bot bot added the cla/signed label Nov 16, 2022
@icinga-probot icinga-probot bot added needs feedback We'll only proceed once we hear from you again TBD To be defined - We aren't certain about this yet labels Nov 16, 2022
@Al2Klimov
Copy link
Member Author

As Tom said

Bildschirmfoto vom 2022-11-16 13-22-19

Comment on lines 247 to 249
At least consider changing the `assign where` filter from, say
`assign where my_complex_filter()`, to `assign where false && my_complex_filter()`.
`&&` will evaluate `false` first and in an instant.
If it's not true, `&&` will "return" `false` and skip `my_complex_filter()`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would you ever want to do that instead of just removing the rule?

Copy link
Member Author

Choose a reason for hiding this comment

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

For keeping it "for later". To disable it temp-ly. IDK.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then you just comment it out? Or is it a Director thing that you can only delete but not disable?

Anyways, this section would be more useful if it covered more cases, like that host.vars.production && match(...) is probably faster than match(...) & host.vars.production.

doc/15-troubleshooting.md Outdated Show resolved Hide resolved
doc/15-troubleshooting.md Outdated Show resolved Hide resolved
doc/15-troubleshooting.md Outdated Show resolved Hide resolved
doc/15-troubleshooting.md Outdated Show resolved Hide resolved
doc/15-troubleshooting.md Outdated Show resolved Hide resolved
doc/15-troubleshooting.md Outdated Show resolved Hide resolved
Comment on lines 247 to 249
At least consider changing the `assign where` filter from, say
`assign where my_complex_filter()`, to `assign where false && my_complex_filter()`.
`&&` will evaluate `false` first and in an instant.
If it's not true, `&&` will "return" `false` and skip `my_complex_filter()`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Then you just comment it out? Or is it a Director thing that you can only delete but not disable?

Anyways, this section would be more useful if it covered more cases, like that host.vars.production && match(...) is probably faster than match(...) & host.vars.production.

doc/15-troubleshooting.md Outdated Show resolved Hide resolved
doc/15-troubleshooting.md Outdated Show resolved Hide resolved
Copy link
Contributor

@julianbrost julianbrost left a comment

Choose a reason for hiding this comment

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

Fine for me it @Thomas-Gelf confirms that the parts relating to Director are fine.

@Al2Klimov
Copy link
Member Author

He already read this at the OSMC and confirmed me that it's tech-ly correct.

@Al2Klimov Al2Klimov removed the TBD To be defined - We aren't certain about this yet label Dec 13, 2022
@Al2Klimov
Copy link
Member Author

Luckily not only he can Director.

@Al2Klimov Al2Klimov removed the request for review from julianbrost January 31, 2023 10:55
@Al2Klimov
Copy link
Member Author

If you don't get your desired feedback once 2.14 approaches, we should just merge this. Enough (patience) is enough.

@Thomas-Gelf
Copy link
Contributor

He already read this at the OSMC and confirmed me that it's tech-ly correct.

When quoting me, please do not pick partial statements. I also told you that while this is technically correct, I consider #2661 being a bad idea, and that I find this explanation more confusing and irritating/scaring, than being helpful.

It reads like an attempt to shift blame for performance issues to our users. What you can read between the lines is "do not use Apply Rules", which doesn't make you feel confident at all. While explaining internal optimizations can be helpful, there is no need to treat the documentation like a Changelog. The documentation itself is versioned.

Also, the "Director Sync workaround to build materialized Apply Rules" example in this PR does only apply if columns in your Import Source carry the very same property name.

@julianbrost
Copy link
Contributor

It reads like an attempt to shift blame for performance issues to our users.

In an ideal world, your Icinga 2 config would be evaluated in the blink of an eye. Until then, we can at least try to provide some guidance on how to get the best out of it right now.

@julianbrost julianbrost merged commit 14d7ee2 into master Feb 3, 2023
@icinga-probot icinga-probot bot deleted the apply-doc branch February 3, 2023 08:46
@Al2Klimov
Copy link
Member Author

there is no need to treat the documentation like a Changelog. The documentation itself is versioned.

Just stumbled over https://docs.python.org/3.9/library/http.client.html#http.client.HTTPSConnection 😎

Changed in version 3.2: source_address, context and check_hostname were added.
Changed in version 3.2: This class now supports HTTPS virtual hosts if possible (that is, if ssl.HAS_SNI is true).
Changed in version 3.4: The strict parameter was removed. HTTP 0.9-style “Simple Responses” are no longer supported.
Changed in version 3.4.3: ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/configuration DSL, parser, compiler, error handling area/documentation End-user or developer help cla/signed enhancement New feature or request needs feedback We'll only proceed once we hear from you again
Projects
None yet
4 participants