Advanced Trigger Filtering Design Proposal #1047

grantr · 2019-04-11T22:54:36Z

Rendered

This document proposes a design for advanced trigger filtering to complete #930.

/cc @bbrowning @Harwayne since I believe you've expressed interest in this topic.

/kind proposal

grantr · 2019-04-11T22:55:49Z

/approve cancel

Removing my approval to ensure another approver signs off.

docs/broker/filtering.md

googlebot · 2019-04-12T23:12:26Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

devguyio · 2019-04-12T23:25:39Z

@grantr can you please rebase? web based commit used wrong email that has no CLA.

(From Ahmed)

googlebot · 2019-04-12T23:36:31Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

grantr · 2019-04-30T18:01:37Z

I've made a few changes to the proposal. Feedback encouraged!

Removed the parseExtensions field. Extensions are always parsed by the SDK, so setting this to false would not actually have an efficiency benefit. The default of false was likely to confuse new users.

Removed the parseData field. The filtering implementation can choose to parse event data only if the data field is used in the expression. This is made possible by CEL's expression parsing and inspection features.

Brought the expression string field up one level. Expressions have one less level of nesting and no longer allow sibling fields.

spec:
  filter:
    expression: ce.type == "com.github.pull.create"

If the need to specify sibling fields returns, we can add a parameterizedExpression filter that allows for them in advanced cases:

### This example is totally made up ###
spec:
  filter:
    parameterizedExpression:
      expression: data.foo.bar == "baz"
      schemaRegistry: "https://example.com/schemas"

Added an attributes map to replace SourceAndType. This can be used to filter any context attribute, including CloudEvents extensions. SourceAndType will be deprecated.

spec:
  filter:
    attributes:
      type: com.github.issue.create
      repository: proposals

Clarified that only one top-level filter is allowed. This eliminates implicit composition semantics which could mislead the user about the filter's output. Top-level composition filters could be added later if we want to enable that functionality:

### This example is for illustration only and is not actually being proposed ###
spec:
  filter:
    and:
    - attributes:
        type: com.github.pull.create
    - expression: ce.source.match("knative")

Thanks everyone for all your feedback so far! Keep it coming. 😄

/milestone v0.6

knative-prow-robot · 2019-04-30T18:01:38Z

@grantr: The provided milestone is not valid for this repository. Milestones in this repository: [v0.6.0]

Use /milestone clear to clear the milestone.

In response to this:

I've made a few changes to the proposal. Feedback encouraged!

Removed the parseExtensions field. Extensions are always parsed by the SDK, so setting this to false would not actually have an efficiency benefit. The default of false was likely to confuse new users.

Removed the parseData field. The filtering implementation can choose to parse event data only if the data field is used in the expression. This is made possible by CEL's expression parsing and inspection features.

Brought the expression string field up one level. Expressions have one less level of nesting and no longer allow sibling fields.
spec:
 filter:
   expression: ce.type == "com.github.pull.create"
If the need to specify sibling fields returns, we can add a parameterizedExpression filter that allows for them in advanced cases:
### This example is totally made up ###
spec:
 filter:
   parameterizedExpression:
     expression: data.foo.bar == "baz"
     schemaRegistry: "https://example.com/schemas"
Added an attributes map to replace SourceAndType. This can be used to filter any context attribute, including CloudEvents extensions. SourceAndType will be deprecated.
spec:
 filter:
   attributes:
     type: com.github.issue.create
     repository: proposals
Clarified that only one top-level filter is allowed. This eliminates implicit composition semantics which could mislead the user about the filter's output. Top-level composition filters could be added later if we want to enable that functionality:
### This example is for illustration only and is not actually being proposed ###
spec:
 filter:
   and:
   - attributes:
       type: com.github.pull.create
   - expression: ce.source.match("knative")
Thanks everyone for all your feedback so far! Keep it coming. 😄

/milestone v0.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

grantr · 2019-04-30T18:09:47Z

/milestone v0.6.0

grantr · 2019-05-01T16:38:12Z

I created a voting document on this proposal in the team drive.

vaikas · 2019-05-02T16:40:13Z

docs/broker/filtering.md

+      user.id == "abc123"
+```
+
+### Exact match on data number field


nit: Header says Exact match, but the example on L372 is a comparison. Given the L362 note, consider changing this to:
data.latency = 300.0

Though, are there rounding errors, or things like that to worry about?

Good catch, I'll fix that.

In CEL numbers are 64 bit integers or doubles. I guess if you're doing arithmetic in the expression you might have to worry about rounding, but doesn't seem relevant if you're just comparing values.

vaikas · 2019-05-02T17:55:23Z

/approve

knative-prow-robot · 2019-05-02T17:55:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vaikas-google

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vaikas-google]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Add open questions related to evaluation errors.

n3wscott · 2019-05-07T02:09:34Z

docs/broker/filtering.md

+[upstream propagation solution](https://github.com/knative/eventing/issues/934)
+for delivery efficiency at scale.
+
+Filters should be easy for the user to specify and simple to interpret at a


Filters should be easy for the user to specify and simple to interpret at a glance.

lol, said regex, never... 🤣

With great power comes great responsibility

n3wscott · 2019-05-07T02:13:44Z

docs/broker/filtering.md

+    - expression: ce.source.match("knative")
+```
+
+## Alternatives Considered


I do not see the alternative of just a function. Did you consider implementing a service service wrapper that implements CEL parser and then use that as a ref in the filter spec? It would use the same mechanize as the subscriptions.

For example, use this entire proposal but make a new trigger type?

TriggerCEL -makes-> a serving.service + a trigger with a function ref. And then promote the CEL syntax up to to the spec level?

I think this would be a useful addition to this section, but I'm a little unclear what UI is being proposed. Can you give an example of the API? Can you briefly describe the benefits and disadvantages from the user's perspective?

mikehelmick · 2019-05-07T20:45:18Z

/lgtm

* Add a proposal for advanced filtering * fix typo in example * Add caveat about variable prefixes * pluralize CloudEvents * fix formatting formatted by formatter * Improve readability of complex expression example * CloudEvents fields are called attributes * Fix table formatting (From Ahmed) * Move extensions to the `ce` prefix This makes extension attributes easier to use and promote to official attributes. The `ext` prefix has been removed. * Expand prefix match example Mention startsWith, match, and matches functions. * Check expression syntax in the Trigger webhook * Add versioning caveat * Reimplement SourceAndType as an expression * Remove parse flags Extensions are always parsed anyway by the CloudEvents SDK, so setting parseExtensions to false has no efficiency benefit. Removing the field improves the approachability of the filtering interface. We can detect when parsing the expression whether data fields are used. This provides a more reliable signal than the parseData flag, so the flag is unnecessary. * Simplify syntax and add attributes filter The `cel` level of the expression filter is removed and `expression` is moved up to its place. An `attributes` filter is added to replace `sourceAndType` (now deprecated). Filter combination at the top level is explicitly disallowed. Examples have been added to clarify some unusual cases. * Note that CloudEvents is vague and in flux CloudEvents is still figuring out whether it wants to support numeric values in extensions. Note that there will probably be dragons for a while. * Clarify structured filters alternative This "alternative considered" is about using structured filters exclusively, without an expression language. Be clear that structured filters are fine as long as they compile down to an expression. * Explain choice of exclusive top-level filter * Add filter spec alternatives considered * Correct inequality example * Add open questions section Add open questions related to evaluation errors.

grantr · 2019-05-13T20:24:06Z

We had some late feedback from teams inside Google suggesting that this proposal might be difficult to implement efficiently at high-scale (for Google and other event providers). I'm recommending that the implementation of this feature be delayed so we can take more time to validate its feasibility for implementers.

grantr · 2019-05-13T20:52:36Z

Also, the voting process indicated disagreement about inclusion of the attributes filter, with approve, neutral, and disapprove votes. I think this requires further usability review and thus should not be included in the 0.6 API.

This reverts commit c0d555a.

Add a proposal for advanced filtering

ae56db9

knative-prow-robot added kind/proposal Issues or PRs related to proposals. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 11, 2019

knative-prow-robot requested review from Harwayne and n3wscott April 11, 2019 22:54

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 11, 2019

knative-prow-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 11, 2019

grantr added 2 commits April 11, 2019 16:04

fix typo in example

1a8ae22

Add caveat about variable prefixes

a35d4a5

grantr commented Apr 11, 2019

View reviewed changes

docs/broker/filtering.md Outdated Show resolved Hide resolved

Harwayne reviewed Apr 11, 2019

View reviewed changes

grantr added 2 commits April 11, 2019 16:13

pluralize CloudEvents

2fe4d27

fix formatting formatted by formatter

ef691e7

Harwayne reviewed Apr 11, 2019

View reviewed changes

docs/broker/filtering.md Outdated Show resolved Hide resolved

grantr added 2 commits April 11, 2019 16:18

Improve readability of complex expression example

d12eb85

CloudEvents fields are called attributes

88c3f86

n3wscott reviewed Apr 12, 2019

View reviewed changes

docs/broker/filtering.md Show resolved Hide resolved

googlebot added cla: no Indicates the PR's author has not signed the CLA. and removed cla: yes Indicates the PR's author has signed the CLA. labels Apr 12, 2019

grantr force-pushed the filtering-proposal branch from c4b2ede to 083f3db Compare April 12, 2019 23:28

Fix table formatting

f40f811

(From Ahmed)

grantr force-pushed the filtering-proposal branch from 083f3db to f40f811 Compare April 12, 2019 23:36

googlebot added cla: yes Indicates the PR's author has signed the CLA. and removed cla: no Indicates the PR's author has not signed the CLA. labels Apr 12, 2019

grantr mentioned this pull request Apr 15, 2019

Cel filters -- Review grantr/eventing#2

Closed

Explain choice of exclusive top-level filter

0b262b8

knative-prow-robot added this to the v0.6.0 milestone Apr 30, 2019

grantr mentioned this pull request May 1, 2019

Upgrade knative/pkg for new stackdriver exporter #1139

Merged

Add filter spec alternatives considered

f7c97b3

vaikas reviewed May 2, 2019

View reviewed changes

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 2, 2019

TristonianJones mentioned this pull request May 2, 2019

List of current implementations and users google/cel-spec#63

Open

grantr added 2 commits May 2, 2019 12:27

Correct inequality example

bcfd13d

Add open questions section

76554b8

Add open questions related to evaluation errors.

grantr mentioned this pull request May 3, 2019

Implement Advanced Filtering proposal #1152

Closed

RichieEscarez mentioned this pull request May 3, 2019

Document advanced trigger filter for eventing knative/docs#1271

Closed

n3wscott reviewed May 7, 2019

View reviewed changes

grantr mentioned this pull request May 7, 2019

[WIP] Eventing Trigger Filtering docs knative/docs#1283

Closed

knative-prow-robot assigned mikehelmick May 7, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 7, 2019

knative-prow-robot merged commit c0d555a into knative:master May 7, 2019

grantr deleted the filtering-proposal branch May 7, 2019 21:46

grantr mentioned this pull request May 7, 2019

adding one more idea to the filter #1176

Closed

grantr added a commit to grantr/eventing that referenced this pull request May 13, 2019

Revert "Advanced Trigger Filtering Design Proposal (knative#1047)"

dbe3ecd

This reverts commit c0d555a.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Advanced Trigger Filtering Design Proposal #1047

Advanced Trigger Filtering Design Proposal #1047

grantr commented Apr 11, 2019 •

edited

grantr commented Apr 11, 2019

googlebot commented Apr 12, 2019

devguyio commented Apr 12, 2019 •

edited

googlebot commented Apr 12, 2019

grantr commented Apr 30, 2019

knative-prow-robot commented Apr 30, 2019

grantr commented Apr 30, 2019

grantr commented May 1, 2019

vaikas May 2, 2019

grantr May 2, 2019

vaikas commented May 2, 2019

knative-prow-robot commented May 2, 2019

n3wscott May 7, 2019

grantr May 7, 2019

n3wscott May 7, 2019

n3wscott May 7, 2019

grantr May 7, 2019

mikehelmick commented May 7, 2019

grantr commented May 13, 2019 •

edited

grantr commented May 13, 2019

Advanced Trigger Filtering Design Proposal #1047

Advanced Trigger Filtering Design Proposal #1047

Conversation

grantr commented Apr 11, 2019 • edited

grantr commented Apr 11, 2019

googlebot commented Apr 12, 2019

devguyio commented Apr 12, 2019 • edited

googlebot commented Apr 12, 2019

grantr commented Apr 30, 2019

knative-prow-robot commented Apr 30, 2019

grantr commented Apr 30, 2019

grantr commented May 1, 2019

vaikas May 2, 2019

Choose a reason for hiding this comment

grantr May 2, 2019

Choose a reason for hiding this comment

vaikas commented May 2, 2019

knative-prow-robot commented May 2, 2019

n3wscott May 7, 2019

Choose a reason for hiding this comment

grantr May 7, 2019

Choose a reason for hiding this comment

n3wscott May 7, 2019

Choose a reason for hiding this comment

n3wscott May 7, 2019

Choose a reason for hiding this comment

grantr May 7, 2019

Choose a reason for hiding this comment

mikehelmick commented May 7, 2019

grantr commented May 13, 2019 • edited

grantr commented May 13, 2019

grantr commented Apr 11, 2019 •

edited

devguyio commented Apr 12, 2019 •

edited

grantr commented May 13, 2019 •

edited