Skip to content

JS expressions in Trigger spec #3783

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.

Sign up for GitHub

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

Closed
slinkydeveloper wants to merge 4 commits into from
Closed

JS expressions in Trigger spec #3783

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cf63ea7
JS as expression language spec
slinkydeveloper Aug 5, 2020
60fca10
Renamed expression to jsExpression
slinkydeveloper Aug 5, 2020
158a50c
Added an expression sample
slinkydeveloper Aug 5, 2020
514aa16
Added clarification about js version supported
slinkydeveloper Aug 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions docs/spec/broker.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,45 @@ default Broker upon creation if no Broker is specified by the user.
A Trigger MAY be created before its assigned Broker exists. A Trigger SHOULD
progress to Ready when its assigned Broker exists and is Ready.

#### Event filters

Implementations of Trigger MUST support both attributes filter and jsExpression
Copy link
Contributor

@n3wscott n3wscott Aug 5, 2020

Choose a reason for hiding this comment

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

I would rather use Rego. https://www.openpolicyagent.org/docs/v0.11.0/language-reference/

It is designed to do this kind of filtering and they have a cross-compiler to web assembly that is stupid fast for filter functions.

Copy link
Contributor

@n3wscott n3wscott Aug 5, 2020

Choose a reason for hiding this comment

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

cc: @grantr

Copy link
Contributor Author

@slinkydeveloper slinkydeveloper Aug 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It is designed to do this kind of filtering and they have a cross-compiler to web assembly that is stupid fast for filter functions.

That's not easy as it seems, I've looked into it: you need a wasm engine, then you need to implement this ABI https://www.openpolicyagent.org/docs/latest/wasm/#exports, then you need to bring the event into the wasm context (which requires a serde step). JS is far easier to integrate and easier also for the users to look at it.

Copy link
Contributor Author

@slinkydeveloper slinkydeveloper Aug 5, 2020

Choose a reason for hiding this comment

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

Then rego brings the non-trivial problem that you need to compile the expressions in wasm modules, invoking the rego compiler, and store them somewhere

Copy link
Contributor

@n3wscott n3wscott Aug 5, 2020

Choose a reason for hiding this comment

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

I don't think it is less trivial than embedding a javascript interpreter, and for this to be able to be well understood we need to define or point to a spec of everything you can do in a jsExpression similar to how Rego lists out all the language features.

Plus we don't need to implement rego, we pull OPA in as a sidecar as one impl.

Copy link
Contributor Author

@slinkydeveloper slinkydeveloper Aug 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think it is less trivial than embedding a javascript interpreter

Look at the code, it's one file of 100 lines of code 😄 And i'm pretty sure it's the very same amount of locs you need in Java to embed a js engine, in rust and so on... All the code to integrate the engine consists in making the event available from the filter code through the engine, then everything else is covered by the js engine itself. Any sidecar solution requires far more, both on sidecar and on filter code + all complexity of configs and tests too. And another point to consider is that this proposal doesn't prevent to have in future rego too.

we need to define or point to a spec of everything you can do in a jsExpression

Not really, I was wondering that all we should say here is: the js engine MUST support EcmaScript 5.1 (which is a requirement every js engine out there supports today) and should serve cloudevent as event (and i think i already covered that part). Maybe we should be more specific on the type coercion between cloudevents types and js types, but overall that's all i think we need here, we don't need to specify all the things you can do in JS

Plus we don't need to implement rego, we pull OPA in as a sidecar as one impl.

That adds quite some overhead to the filtering process and complexity too... I prefer to have something simple that I can embed straight in my code, other solutions that requires some external process are more complex to implement and most notably adds overhead for serializing/deserializing and IPC.

Copy link
Contributor

@nachocano nachocano Aug 6, 2020

Choose a reason for hiding this comment

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

I'm not convinced on the "MUST support both attributes filter and jsExpression".

I actually like the filter dialects the CloudSubscriptions spec talks about. See https://github.com/cloudevents/spec/blob/master/subscriptions-api.md#323-filter-dialects.
The MUST is to support a basic one (Exact, Prefix and Suffix match).
More advanced dialects can be made optional. I can easily see JS being an optional advanced dialect. Maybe REGO, CEL, or whatever, other ones, and so on...

Overall, I think you will be able to reduce contention if you decouple Trigger from a mandatory JS "dialect", and just frame the problem as Trigger with optional dialects... I think conversations will move forward faster, and we will also be more aligned with the Serverless WG (which in general I think is a good thing).

Copy link
Contributor Author

@slinkydeveloper slinkydeveloper Aug 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

TBH I wasn't aware of these filter dialects from subscriptions spec. Still they don't seem to cover basic nested logic and operations on types (in this doc i explain what a user should be able to express https://docs.google.com/document/d/1Pz2vaLWKUrMQDLNyDW7ksjgLG4GAxBY546dT7_jF5Rk/edit?usp=sharing)

My take on optional and mandatory is that, I have no strong opinion if vendors wants to add their filtering capabilities to their system, but IMHO eventing vanilla should be bundled with a filtering capability that covers a 90% of basic use cases. Not providing a "good enough" bundled filtering capability will make too big the divergence between different implementations (basically, if a user wants to port trigger a to another vendor, he needs to rewrite all the filtering logic...)

JS "dialect"

I hardly name it a JS dialect: In the spec here i'm just stating that the expression is ECMAScript 5.1+ and you access to event using event.attrx, I was very careful to avoid defining something that cannot trivially implemented with an already existing js engine.

Copy link
Contributor

@n3wscott n3wscott Aug 11, 2020

Choose a reason for hiding this comment

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

I would rather look at what is missing in the subscription spec and upstream changes and take what is there for now.

Copy link
Contributor

@n3wscott n3wscott Aug 11, 2020

Choose a reason for hiding this comment

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

crazy thought... what if you proposed a javascript dialect to the spec https://github.com/cloudevents/spec/blob/master/subscriptions-api.md#323-filter-dialects

filter.

The attributes filter specifying a list of key-value pairs MUST be supported by
Trigger. Events that pass the attributes filter MUST include context or
extension attributes that match all key-value pairs exactly.

The jsExpression filter specifying a Javascript expression MUST be supported by
Trigger. Implementations MUST support evaluation of Javascript expressions
following the standard
[EcmaScript 5.1](http://www.ecma-international.org/ecma-262/5.1/), although they
MAY support [EcmaScript 6](http://www.ecma-international.org/ecma-262/6.0/) or
greater. If the evaluation of the expression returns a Javascript value
coercible to the boolean `true`, then the trigger filter MUST pass the event.
Implementations MUST make the event accessible, within the expression, through
the variable `event` and its context attributes and extensions through the
Javascript dot notation `.`. For example the `id` attribute of a CloudEvent
should be accessible with `event.id`. Implementations MUST coerce CloudEvent
types to JS types. For example the value of an attribute with type `Timestamp`
should be translated to the JS type `Date`.

An example jsExpression is:

```js
event.id.indexOf("francesco") != -1 &&
event.time != null &&
event.time.getFullYear() >= 2020 &&
event.exta != null;
```

This expression MUST pass the event iff the event id contains the string
`francesco`, event time has a year greater or equal to 2020 and event contains
an extension named `exta`.

If a Trigger specify both attributes and jsExpression filters, then the event
MUST pass only and only if both filters pass the event.

## Data Plane

### Ingress
Expand Down
7 changes: 4 additions & 3 deletions docs/spec/spec.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ retrieved from ref.

### TriggerFilter

| Field | Type | Description | Constraints |
| ---------- | ----------------- | ----------------------------------------------------------------------------------- | ----------- |
| attributes | map[string]string | A filter specifying which events match this trigger. Matches exactly on the fields. | |
| Field | Type | Description | Constraints |
| ------------ | ----------------- | ------------------------------------------------------------------------------------------------------------------------- | ----------- |
| attributes | map[string]string | A filter specifying which events match this trigger. Matches exactly on the fields. | |
| jsExpression | string | Javascript expression used for filtering events. If not specified, the behaviour is the same as an expression always true | |