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

Calculate time spent on evaluating mismatching apply rules and parent objects #9580

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

Conversation

to avoid malloc(). This BREAKS assign where locals.x = 1.
@Al2Klimov Al2Klimov self-assigned this Nov 11, 2022
@cla-bot cla-bot bot added the cla/signed label Nov 11, 2022
@icinga-probot icinga-probot bot added area/configuration DSL, parser, compiler, error handling core/quality Improve code, libraries, algorithms, inline docs labels Nov 11, 2022
@julianbrost
Copy link
Contributor

Is this just for testing or something you actually want to merge?

@Al2Klimov
Copy link
Member Author

At least I consider it useful for the customer or NET colleague to figure out which apply rule to start with while doing #9585.

@Al2Klimov
Copy link
Member Author

Do you agree?

@Al2Klimov Al2Klimov removed their assignment Jan 31, 2023
@Al2Klimov Al2Klimov added this to awaits concept review (i.e. TBD) in 42 quadrillions PRs Jan 31, 2023
@lippserd
Copy link
Member

Is this really just measuring time for mismatched apply rules that should be removed anyway if considered invalid by the user? If so, I am in favor of closing this PR.

@julianbrost
Copy link
Contributor

The diff is huge compared to what I'd expect just from the PR title, so I don't know what this PR actually does. My initial impression was that this might be something that was built for debugging something, but if you think this would be useful in a release, please explain what it does exactly.

@Al2Klimov Al2Klimov moved this from awaits concept review (i.e. TBD) to blocked in 42 quadrillions PRs Jan 31, 2023
@Al2Klimov
Copy link
Member Author

The never-matched apply rules are already listed. This one targets apply rules which matched, but not on all parent objects. They're sorted descending by total time spent on filter executions which resulted in a mismatch divided by mismatched parent objects.

E.g. your apply rule has a very complex (so long-running) assign condition. It matches a few of your army of hosts. But it evals for all of them. The latter wastes lots of time. This is bad. So this assign condition is bad. This PR lists the rules with the worst assign conditions. But there's still some TODO anyway, so no reason to already review.

@julianbrost
Copy link
Contributor

This one targets apply rules which matched, but not on all parent objects.

Sounds like just the typical use-case of apply rules to me.

They're sorted descending by total time spent on filter executions which resulted in a mismatch divided by mismatched parent objects.

Therefore, this sounds like this would just generate a lot of output. And users are ignoring "your rule didn't match at all" warnings already.

@Al2Klimov
Copy link
Member Author

Therefore, this sounds like this would just generate a lot of output.

In the debug log.

And users are ignoring "your rule didn't match at all" warnings already.

Until they complain to the support about our speed and we read their logs.

@julianbrost
Copy link
Contributor

So if I had a service apply rule that matched on 70% of my hosts, this would tell me the time that was spent on evaluating this rule on the other 30%? This sounds like a strange metric to me. If an apply rule generates more objects, it's fine if it's more expensive to evaluate? The extreme case would be a super expensive to evaluate rule that creates services for all hosts and this wouldn't show up anywhere as there were no mismatches?

In general, I doubt the general usefulness of this change. Sure, it might be helpful in a few situations to figure something out, but this looks more like a hidden feature that little users would even be aware of and for this, it would need quite a big code change.

@Al2Klimov
Copy link
Member Author

Hey, with this I discovered #9585. But yes, we don't have to include this.

@Al2Klimov
Copy link
Member Author

Would you prefer if I don't separate those 70/30%, i.e. look at the 100% (still divided by parent candidates)?

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 cla/signed core/quality Improve code, libraries, algorithms, inline docs
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants