Skip to content

Implement Advanced Filtering proposal#1152

Closed
grantr wants to merge 10 commits into
knative:masterfrom
grantr:advanced-filtering
Closed

Implement Advanced Filtering proposal#1152
grantr wants to merge 10 commits into
knative:masterfrom
grantr:advanced-filtering

Conversation

@grantr
Copy link
Copy Markdown
Contributor

@grantr grantr commented May 3, 2019

Implements #1047 at 76554b8.

Proposed Changes

  • Add attributes filter to the Trigger spec accepting a map of attribute keys and values to compare to the event context attributes, including extensions. Replaces sourceAndType.
  • Add expression filter to the Trigger spec accepting a CEL expression to be evaluated with respect to the event context and data.
  • Deprecate sourceAndType filter. This filter may be removed or translated into an attributes filter in a future release.
  • Validate that only a single filter strategy is selected per Trigger.

Details

Only one filter can be specified per Trigger. The Trigger will be rejected as invalid if multiple filters are specified. If no filters are specified, that's currently interpreted as a filter matching everything.

CEL expressions are compiled into programs and cached in an LRU cache by expression string. Currently the cache size is fixed at 100 entries, though that should eventually be based on the number of Triggers.

Expressions are inspected to determine if they rely on parsing event data. Event data is not parsed when the expression doesn't need it.

The SourceAndType and Attributes filters are implemented by translation to CEL expression.

Attribute filter keys containing special characters such as . are supported.

Attribute filters do not support comparing non-string values. Attribute filters containing non-string values will be TODO. Non-string attribute values compared to attribute filters will generate an evaluation error.

Evaluation errors cause events to be rejected.

Remaining work

Currently evaluation errors are only surfaced in dispatcher logs. This should be surfaced in a form more visible to the user.
Instrumentation is missing and should be added before release.

Release Note

- Add `attributes` filter to the Trigger spec accepting a map of attribute keys and values to compare to the event context attributes, including extensions. Replaces `sourceAndType`.
- Add `expression` filter to the Trigger spec accepting a CEL expression to be evaluated with respect to the event context and data.
- Deprecate `sourceAndType` filter. This filter may be removed or translated into an `attributes` filter in a future release.
- Validate that only a single filter strategy is selected per Trigger.

/hold
Holding for Docs PR and #1047 merge.

Fixes #930.

grantr added 2 commits May 2, 2019 17:22
The Attributes filter is a map of attribute keys to values to compare for
equality with the corresponding event context attributes, including
extensions. This replaces the SourceAndType filter which is now
deprecated.

The Expression filter is a string expression that is evaluated by the
CEL runtime for each event's context and data.

Only one filter can be specified per Trigger. The Trigger will be
rejected as invalid if multiple filters are specified. If no
filters are specified, that's currently interpreted as a filter
matching everything.
Add the CEL runtime to filter CEL expressions. CEL expressions are
compiled into programs and cached in an LRU cache by expression string.
Currently the cache size is fixed at 100 entries, though that should
eventually be based on the number of Triggers.

Expressions are inspected to determine if they rely on parsing event
data. Event data is not parsed when the expression doesn't need it.

The SourceAndType and Attributes filters are implemented by translation
to CEL expression.
@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 3, 2019
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 3, 2019
@knative-prow-robot knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 3, 2019
@knative-prow-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grantr

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 3, 2019
grantr added 8 commits May 3, 2019 12:30
This is an error today, but it may not be in the future, depending on
how strict users want to be with types.
The receiver now records an overall event received count per trigger,
with accept/reject result tags, and a filtering time histogram with
pass/fail/error result tags.
Same setup as the broker ingress pod: OpenCensus Prometheus exporter
listening on :9090.
@grantr
Copy link
Copy Markdown
Contributor Author

grantr commented May 6, 2019

The receiver now instruments filtering. This is ready for review.

@knative-metrics-robot
Copy link
Copy Markdown

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/apis/eventing/v1alpha1/trigger_validation.go 100.0% 97.4% -2.6
pkg/broker/expression_filter.go Do not exist 87.2%
pkg/broker/metrics.go Do not exist 71.4%
pkg/broker/program_cache.go Do not exist 83.3%
pkg/broker/receiver.go 82.9% 87.3% 4.3
pkg/reconciler/trigger/trigger.go Do not exist 83.4%

@grantr
Copy link
Copy Markdown
Contributor Author

grantr commented May 13, 2019

This is on hold for now; see #1047 (comment).

@grantr
Copy link
Copy Markdown
Contributor Author

grantr commented Aug 29, 2019

Attributes filters made it in via #1643.

matzew added a commit to matzew/eventing that referenced this pull request Apr 1, 2021
…tive#1152)

Co-authored-by: Yi Zhang <cathyzhyi@google.com>

Co-authored-by: Knative Prow Robot <knative-prow-robot@google.com>
Co-authored-by: Yi Zhang <cathyzhyi@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Advanced Filtering on Triggers

4 participants