Skip to content

Conversation

@Kami
Copy link
Member

@Kami Kami commented May 18, 2018

This pull request updates the code to also create RuleEnforcementDB object for trigger instances which resulted in an exception and failed to match the rule in the rules filtering (matching) phase.

This could for example happen if rule contains an invalid Jinja expression and similar.

This change increases the user visibility. Previously it was only possible to find about those trigger instances and rules by checking the rules engine log file.

Some people could argue that this doesn't really belong into RuleEnforcementDB object per say and we should have a separate object for that (e.g. RuleFilteringFailureDB or similar), but I think this would increase the complexity for no good reason (and StackStorm is already quite complex and we already have quite a lot of models).

Keep in mind that we only create RuleEnforcementDB object for rules which result in an exception - we don't create RuleEnforcementDB objects for trigger instances which didn't match any rules due to simple criteria match failure (that would be an inverse for RuleEnforcementDB object and I think I have a better idea for handling that below).

This change was originally requested by @dzimine / @enykeev. After I looked into the code, I agreed is a good idea since it increases the user visibility into the rules matching.

To increase visibility further, I would suggest to have another API endpoint or similar which returns a list of TriggerInstanceDB objects that didn't match any rule.

This could be implement quite easily by checking for all TriggerInstanceDB objects in the system which don't have corresponding RuleEnforcementDB objects. Keep in mind that this could be quite expensive (there could be many TriggerInstanceDB objects in the database) so we should keep performance in mind.

TODO

  • Tests

Kami added 2 commits May 18, 2018 11:08
to match rule inside the rules filtering phase due to an exception.

This way user has more visibility into matches which failed because of
an exception (failed to render Jinja expression, etc).

Without this change, only way to find those rules is by checking rules
engine service log.
@Kami Kami added this to the 2.8.0 milestone May 18, 2018
@Kami Kami force-pushed the rule_enforcement_improvements branch from 70f7742 to 9c11d01 Compare May 18, 2018 19:45
Kami added 7 commits May 18, 2018 21:49
quoted when projection is used on a query (aka only want to retrieve a
subset of fields or similar).
rule enforcement objects enriched with additional data useful to the
WebUI and various clients (right now that's execution parameters and
action ref).
corresponding trigger instance object.
@Kami
Copy link
Member Author

Kami commented May 22, 2018

I discussed this with @enykeev some more and we agreed it would be good to add some new rule enforcement view API endpoints:

  • GET /v1/ruleenforcements/view/
  • GET /v1/ruleenforcements/view/?rule_ref=mypack.my_rule
  • GET /v1/ruleenforcements/view/<enforcement id>

Example response format can be viewed here - https://gist.github.com/Kami/07a061b5261b0064c23962bbe1f50dff.

Similar as other view API endpoints, this API endpoint returns aggregated data from multiple objects which reduces load and unnecessary HTTP requests on the client side.

In this case, the endpoint returns RuleEnforcement object with the corresponding TriggerInstance and Execution object (in case trigger instance resulted in an action execution). TriggerInstance object is small so the whole object is included, but Execution can be quite large so right now only the fields which are needed by the UI are included (action reference and action input parameters).

Before this change, WebUI would need to make three HTTP requests to assemble "single rule expanded pane view" page (get rule enforcement, get trigger instance, get execution), but now it only needs to make one.

In the end it reduces load on the client and provides a better and faster user-experience. Keep in mind that this API endpoint can be used by other clients which want to provide more detailed views, not just WebUI.

@Kami Kami requested a review from enykeev May 22, 2018 15:22
Kami added 2 commits May 22, 2018 17:28
When this argument is True, we use object ids (primary keys) from
fixture files when saving objects in the database.

This allows us to re-create reproducible tests in cases when tests rely
on fixed ids without needing to mock a lot of things (mocking is error
prone and not robust in a lot of scenarios).
@Kami
Copy link
Member Author

Kami commented May 23, 2018

Here is how the output looks right now with some additional fields (action.parameters, runner.name, runner.runner_parameters, status) added - https://gist.github.com/Kami/501ba43d011e0f54b02f60e322734df9

@Kami
Copy link
Member Author

Kami commented May 24, 2018

As per discussion with @enykeev, I also added new status field to the RuleEnforcementDB model.

This field can contain the following values:

  • succeeded - trigger instance matched a rule and as a result action execution was successfully scheduled
  • failed - trigger instance matched a rule, but action execution was not schedule due to an exception in the scheduling / param rendering / etc phase (e.g. Jinja exception when trying to render parameters inside the rule, etc.)

For backward compatibility reasons, default value for this field is succeeded. Before this PR, there was no concept / way to track failed enforcement besides the service log file.

Copy link
Member

@enykeev enykeev left a comment

Choose a reason for hiding this comment

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

LGTM

@Kami Kami merged commit 442f920 into master May 24, 2018
@Kami Kami deleted the rule_enforcement_improvements branch May 24, 2018 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants