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

Provide an alternative expression plugin #800

Conversation

chicco785
Copy link
Contributor

This pull request provide a new plugin to integrate JEXL (https://github.com/TomFrost/jexl), a library that provides a rich javascript expression language, as an alternative to the native IoTAgent Lib expression language.

@chicco785
Copy link
Contributor Author

see #801

@chicco785 chicco785 force-pushed the expression-lib-alternative branch 2 times, most recently from 93105b2 to 545dfb3 Compare August 5, 2019 16:16
@chicco785
Copy link
Contributor Author

with reference to #687 Jexl supports:

  • if(cond,then,else) function vai ternary operator
  • Comparison operators: <, <=, >, <=
  • Logical operators: and, or, not

Most probably the following functions can be easily introduced:

  • Bitwise operations: <<, >>, and, or, not
  • Math functions: sin, cos, abs, min, man, random, mod, floor, ceiling, round
  • String functions: uppercase, lowercase, replace, slice

And interestingly it allows "Object" navigation, e.g., supposing the context is

{
  name: {
    first: "Malory",
    last: "Archer"
  },
  exes: [
    "Nikolai Jakov",
    "Len Trexler",
    "Burt Reynolds"
  ],
  lastEx: 2
}

operation navigating the object are possible:

name.first

@chicco785 chicco785 changed the title Provide an alternative expression plugin WIP: Provide an alternative expression plugin Aug 5, 2019
@chicco785 chicco785 force-pushed the expression-lib-alternative branch 3 times, most recently from 0981862 to f223691 Compare August 5, 2019 17:01
@chicco785 chicco785 changed the title WIP: Provide an alternative expression plugin Provide an alternative expression plugin Aug 5, 2019
@fgalan
Copy link
Member

fgalan commented Aug 6, 2019

It seems promising. Thanks for your contribution!

We'll have a closer look, but a first question I have, please. Are the changes backward compatible? I mean, the existing expressions would keep working if this work is incorporated to IOTA Lib? It seems you have commented out some existing unit test so I'm not fully sure...

@chicco785
Copy link
Contributor Author

@fgalan i am afraid they are not, my idea was to enable one version or the other using a flag in the configuration file. a grammar "converter" may be able to solve the issue, but I am not expert on that. i created new tests that mimic the old tests, but where expression are re-written in the new language. if I comment out tests, it is my mistake (it was to speed up the testing)

main differences in the grammar based on my experience with the rewriting of tests:

  • @ is not used to distinguish a variable
  • an expression like trim(var) is converted to var|trim
  • you need to be explicit about string concatenation (afaik)

So it could be that indeed it could be possible to rewrite easily most of the expressions to the new lib. I won't have time to work on that now, but also, I think it could be a separate PR.

@chicco785
Copy link
Contributor Author

at the time being i am testing it with lorawan, and managed the flag based configuration here:
https://github.com/Atos-Research-and-Innovation/IoTagent-LoRaWAN/pull/78/files#diff-783de7b0938d2b5641708a1e0a60cb45R633

the flag support could be as well moved in directly in the lib and totally removed if the grammar option works out

@fgalan
Copy link
Member

fgalan commented Aug 6, 2019

To clarify, the problem with backward compatibility is not in the sense the new grammar cannot cover the existing other (probably the one you proposes is richer, so the existing one can be rewritten in terms of yours). The problem is that existing provisioned devices using the existing grammar will fail when the new one is enabled. Re-provisioning devices is not always possible when you have a large deployment with hundreds of devices.

@chicco785
Copy link
Contributor Author

@fgalan understood that, in fact i was talking about "grammar" conversion in the sense that:
-> old expression is parsed
-> converted to new grammar
-> executed

but as said i think could be a secondary pr, and alternative could be also a db update script.

@mrutid
Copy link
Member

mrutid commented Aug 6, 2019

As Fermin states, it is so important to provide a mechanism so both expressions may coexist in a given deployment (not just to choose one or another with a flag). We like your approach and the new functionality but as far as it implies to modify the whole set of production provisioned devices It can't be used in those environments. If both methods could coexist we could manage an ordered deprecation of the old expression plugin and ulterior migration, but we can't merge otherwise.

@mrutid
Copy link
Member

mrutid commented Aug 6, 2019

Maybe it's just a matter to employ a new key in the device JSON, so "expression" will use the current language expression, while "JEXLexpresison" contains the new one. In case both are present I see no problem in considering "JEXLexpresison" as the preferred.

@chicco785
Copy link
Contributor Author

also that is an option, but i tend to think that having a way to parse old expression to the new language would be more interesting and user friendly. still this may not be trivial, and I am afraid (having you probably the largest existing set of cases to test) a lot of help on your side may be needed.
in general, still i don't see why it could not be an option to incorporate the change with "configuration" flag approach till the conversion is not implemented:

  • people that don't want to change the configuration of their devices will set useJEXL = false;
  • people who don't have devices to update or are keen to update will set useJEXL = true;

@mrutid
Copy link
Member

mrutid commented Aug 6, 2019

"in general, still i don't see why it could not be an option to incorporate the change with "configuration" flag approach till the conversion is not implemented:"

Because we (and nobody playing for real with Agents) won't be able to migrate the whole production ecosystem at once to the new expression schema (while guaranteeing SLA and reliability 99,7%). So we won't be able to put the flag useJEXL = true never ever. And we really would love to use it. The only possible way is coexistence, so newly provisioned devices may use the new method, and we can take time to manage the "legacy" migration of all the different clients and projects.

We understand the inconvenience and the extra complexity, but it is mandatory to take care of existing users and projects in order to continue evolving.

BTW thanks for the nice contribution.

@chicco785
Copy link
Contributor Author

  • "So we won't be able to put the flag useJEXL = true never ever. And we really would love to use it."
    you didn't follow my reasoning :) and also you didn't let me know if you are interested in helping, because as said, the 99% sla ect for the conversion, requires to have proper tests, which are not available to day, and probably only you can provide :)

@mrutid
Copy link
Member

mrutid commented Aug 6, 2019

Sorry if I misunderstood you. Probably I'm running too much with this thread and I have only 3% of CPU available. Please try to follow our reasoning also.

Are you proposing to accept both expression languages in the same placeholder and make an automatic translation? or are you proposing some kind of batch processing? I see that options far more complex and risky than allowing both mechanism as separated functionalities.
The way we are proposing makes the upgrade to the new IotAgent version a lot less critical and will empower and simplify the usage of the new functionality (the point is to be able to experiment with it as soon as possible in current deployments without the need of substitute or make big changes).

For Example the FLAG Option was currently explored in Cygnus in order to migrate Grouping Rules to NameMappings ("xor" functionalities managed with a flag) and believe in me it is being complex, slow and painful.

We are always willing to help, but we don't have infinite resources and it's not easy to allocate them at features that are not currently demanded by our customers. As I said we like your proposal, and we will do our best to be able to incorporate it without incurring into extra risk.

Best.

@chicco785
Copy link
Contributor Author

I understand the extra risk about the grammar conversion approach. I thought it could empower more easily old users.

What about:
1 - there is a configuration flag that defines the default expression language
2 - alternatively, to specify the expression language to be used (if not happy with the default), the user can set a field "expression-language" to its favourite one expression by expression. (i would avoid to have different expression container fields)

@mrutid
Copy link
Member

mrutid commented Aug 6, 2019

I think now you are not following our reasoning ;)

It's not only a problem with the API old users themselves (I think they would like the new expression language). It's a matter of current/old provisioned devices, we have thousands of devices from different projects, domains, customers and integrators, some of which has expression associated.

If the deployment of the new IoTAgent release makes my MongoDB data model incompatible, and a complex data migration/transformation process is needed (including parsing and transpiling!) it will make the upgrade difficult & risky. In fact not viable in the short/mid-term.

That's why an incremental approach is needed, we need to avoid data model incompatibility between consecutive releases. So from our point of view, it will be much more interesting to embrace a coexistence/deprecation mechanism. The same successful approach was followed by NGSIv1 and v2.

The different containers it's not so bad option, maybe not the most elegant, but it seems cheap and highly operative.

@chicco785
Copy link
Contributor Author

If the deployment of the new IoTAgent release makes my MongoDB data model incompatible, and a complex data migration/transformation process is needed (including parsing and transpiling!) it will make the upgrade difficult & risky. In fact not viable in the short/mid-term.

I got that, in fact this was the reason why for the "grammar automatic translation" approach, i mentioned that big set of test examples would be needed. I thought this was the preference by @fgalan to support backward compatibility, that's why I originally proposed that.

The different containers it's not so bad option, maybe not the most elegant, but it seems cheap and highly operative.

Not sure if your refer to your original idea JEXLexpression + expression or my proposal
expression + expression-language. I think this second option is more elegant and more flexible. You would have flag that defines the default language, so that you don't need to use every time expression-language, but only when needed. Progressive migration of devices configuration would be more simple, you would update expression and set expression-language. Also tools / scripts that today use expression field to create payloads won't need to be changed (except eventually for the parsing part)

@mrutid
Copy link
Member

mrutid commented Aug 6, 2019

I think now I got you. Sorry, I'm slow... and my english doesn't help. Your last proposal seems pretty good (I were thinking about config options in your proposal instead of mongo docs)

What about:
1 - there is a configuration flag that defines the default expression language
2 - alternatively, to specify the expression language to be used (if not happy with the default), the user can set a field "expression-language" to its favourite one expression by expression. (i would avoid to have different expression container fields)

If 1 - defaults to "OLD" if flag is not present I think it covers perfectly with our requirements.

@chicco785
Copy link
Contributor Author

If 1 - defaults to "OLD" if flag is not present I think it covers perfectly with our requirements.

sure, that would be the default till you don't decide to deprecate the old one

@chicco785
Copy link
Contributor Author

If 1 - defaults to "OLD" if flag is not present I think it covers perfectly with our requirements.

sure, that would be the default till you don't decide to deprecate the old one

@fgalan @mrutid let me know if i should proceed with the plan!

@mrutid
Copy link
Member

mrutid commented Aug 7, 2019

Ok for me. Fermin is not available till next week, it's always good to have his feedback... but go ahead if you are in a hurry.

@chicco785
Copy link
Contributor Author

chicco785 commented Aug 7, 2019 via email

@fgalan
Copy link
Member

fgalan commented Aug 14, 2019

I'm back :)

There has been a long iteration between @mrutid and @chicco785 but I understand the final conclusion is this:

1 - there is a configuration flag that defines the default expression language
2 - alternatively, to specify the expression language to be used (if not happy with the default), the user can set a field "expression-language" to its favourite one expression by expression. (i would avoid to have different expression container fields)

Based on that, I'll provide the following precise proposal:

For item 1, the following configuration parameter:

defaultExpressionLanguage: legacy|jexl

Associated env var: IOTA_DEFAULT_EXPRESSION_LANGUAGE

Default (if the parameter is not present) is "legacy", to keep backward compatibility with existing deployment.

For item 2, a "expressionLanguage" field uses at device provision time. If not present, then the language in configuration parameter defaultExpressionLanguage is the one to be used.

I see two alternatives for "expressionLanguage":

2a) Per expression language setting:

{
   "devices":[
      {
         "device_id":"45",
         "protocol":"GENERIC_PROTO",
         "entity_name":"WasteContainer:WC45",
         "entity_type":"WasteContainer",
         "attributes":[
            {
               "name":"location",
               "type":"geo:point",
               "expression": "...",
               "expressionLanguage": "legacy"
            },
            {
               "name":"fillingLevel",
               "type":"Number",
               "expression": "...",
               "expressionLanguage": "jexl"
            }
         ]
      }
   ]
}

2b) Per device language setting:

{
   "devices":[
      {
         "device_id":"45",
         "protocol":"GENERIC_PROTO",
         "entity_name":"WasteContainer:WC45",
         "entity_type":"WasteContainer",
         "expressionLanguage": "legacy",
         "attributes":[
            {
               "name":"location",
               "type":"geo:point",
               "expression": "..."
            },
            {
               "name":"fillingLevel",
               "type":"Number",
               "expression": "..."
            }
         ]
      }
   ]
}

The last one is simpler in the case the user doesn't mix languages in the same device (which seems to be a reasonable assumption).

@chicco785 what do you think? Could yo modify your PR according that proposal? Thanks!

| '" a "|trim' | 'a' |
| 'name|length' | 8 |
| 'name|indexOf("e")' | 1 |
| 'name|substring(0,name|indexOf("e")+1)'| 'De' |
Copy link
Member

Choose a reason for hiding this comment

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

This one seems also problematic.

Copy link
Contributor Author

@chicco785 chicco785 Feb 11, 2020

Choose a reason for hiding this comment

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

fix by 042c9fd

@chicco785 chicco785 force-pushed the expression-lib-alternative branch from d829d42 to 042c9fd Compare February 11, 2020 14:15
@chicco785
Copy link
Contributor Author

chicco785 commented Feb 11, 2020

A quick question, we will need to be able to apply DateTime transformation expressions as part of the expression plugging (i.e: add a minute to a given Date). How could this be addressed with JEXL?

This behavior can be easily added to the current expresión language, but if there is a complete support of this functionality in JEXL it could be a nice driver to deprecate the curren expresión language in favor of JEXL.

JEXL is not natively supporting dates, anyhow there are some clues on how to manage that:
TomFrost/Jexl#49
TomFrost/Jexl#23

ideally we could "test" a value to be a Date in this function:
https://github.com/orchestracities/iotagent-node-lib/blob/expression-lib-alternative/lib/plugins/jexlParser.js#L66

and the add a function to add a given time unit.
beyond that, I would think of also of data conversion functions such as toEpoch fromEpoch,
as commented to @fgalan this should be another PR

Copy link
Member

@fgalan fgalan left a comment

Choose a reason for hiding this comment

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

LGTM, at commit 3b6824b (although a small conflict with CHANGES_NEXT_RELEASE has to be solved). Thanks for the contribution!

Now we are going to conduct a CI test on this branch and some additional manual testing of the new feature. If everything goes ok, the PR will be merged :)

@fgalan
Copy link
Member

fgalan commented Feb 18, 2020

Now we are going to conduct a CI test on this branch and some additional manual testing of the new feature. If everything goes ok, the PR will be merged :)

These are the branches in the IOTAs' repositories to test regression (the will be deleted once regression finishes):

telefonicaid/iotagent-ul#418
telefonicaid/iotagent-json#467

@fgalan
Copy link
Member

fgalan commented Feb 20, 2020

I'm doing some testing of the functionality but I'm getting problems...

First, this is the way I deploy the setup. Basically the same docker-compose.yml described here https://hub.docker.com/r/telefonicaiot/iotagent-ul/, except the image of the agent (telefonicaiot/iotagent-ul:jexl points to a image generated using telefonicaid/iotagent-ul#418)

version: "3.1"

volumes:
    mongodb: ~ 

services:
    iot-agent:
        image: telefonicaiot/iotagent-ul:jexl
        hostname: iot-agent
        container_name: fiware-iot-agent
        depends_on:
            - mongodb
        expose:
            - "4061"
            - "7896"
        ports:
            - "4061:4061"
            - "7896:7896"
        environment:
            - "IOTA_CB_HOST=orion"
            - "IOTA_CB_PORT=1026"
            - "IOTA_NORTH_PORT=4061"
            - "IOTA_REGISTRY_TYPE=mongodb"
            - "IOTA_MONGO_HOST=mongodb"
            - "IOTA_MONGO_PORT=27017"
            - "IOTA_MONGO_DB=iotagent-ul"
            - "IOTA_HTTP_PORT=7896"
            - "IOTA_PROVIDER_URL=http://iot-agent:4061"

    mongodb:
        image: mongo:3.6
        hostname: mongodb
        container_name: db-mongo
        ports:
            - "27017:27017"
        command: --bind_ip_all --smallfiles
        volumes:
            - mongodb:/data

    orion:
        image: fiware/orion
        hostname: orion
        container_name: fiware-orion
        depends_on:
            - mongodb
        expose:
            - "1026"
        ports:
            - "1026:1026"
        command: -dbhost mongodb

Now, service provision and device provision. Note the expression used (level * 100).

curl -iX POST \
  'http://localhost:4061/iot/services' \
  -H 'Content-Type: application/json' \
  -H 'fiware-service: openiot' \
  -H 'fiware-servicepath: /' \
  -d '{
 "services": [
   {
     "apikey":      "4jggokgpepnvsb2uv4s40d59ov",
     "cbroker":     "http://orion:1026",
     "entity_type": "Thing",
     "resource":    "/iot/d"
   }
 ]
}'

curl -iX POST \
  'http://localhost:4061/iot/devices' \
  -H 'Content-Type: application/json' \
  -H 'fiware-service: openiot' \
  -H 'fiware-servicepath: /' \
  -d '{
 "devices": [
   {
         "device_id": "device01",
         "entity_name": "device01",
         "entity_type": "Thing",
         "expressionLanguage": "jexl",
         "attributes":[
            {
               "name":"fillLevel",
               "type":"Number",
               "expression": "level * 100"
            }
         ]
   }
 ]
}
'

Now I send a measure in the southbound interface (level equal to 2, expecting fillLevel equal to 200)

curl -iX POST \
  'http://localhost:7896/iot/d?k=4jggokgpepnvsb2uv4s40d59ov&i=device01' \
  -H 'Content-Type: text/plain' \
  -d 'level|2'

However what I get is this error message:

TTP/1.1 500 Internal Server Error
X-Powered-By: Express
Content-Type: application/json; charset=utf-8
Content-Length: 67
ETag: W/"43-q0I6iDJp4QcjMhOybf4KsPGAScM"
Date: Thu, 20 Feb 2020 14:51:13 GMT
Connection: keep-alive

{"name":"TypeError","message":"Cannot read property 'map' of null"}

@chicco785 could you have a look, please? Maybe I'm doing something incorrectly?

@fgalan
Copy link
Member

fgalan commented Feb 21, 2020

Taking into account that the legacy expression functionality requires the variables to be used in the expression to be provisioned as attributes (otherwise, you will get a {"name":"ATTRIBUTE_NOT_FOUND","message":"Some of the attributes does not exist"} response to the request sending the measures in the southbound) I have repeated the test using this device provision request instead the one above (adding level):

curl -iX POST \
  'http://localhost:4061/iot/devices' \
  -H 'Content-Type: application/json' \
  -H 'fiware-service: openiot' \
  -H 'fiware-servicepath: /' \
  -d '{
 "devices": [
   {
         "device_id": "device01",
         "entity_name": "device01",
         "entity_type": "Thing",
         "expressionLanguage": "jexl",
         "attributes":[
            {
               "name":"fillLevel",
               "type":"Number",
               "expression": "level * 100"
            },
            {
               "name":"level",
               "type":"Number"
            }
         ]
   }
 ]
}

It's still failiing, although now the response I got when I send measures is different (and doesn't have too much sense, from my point of view):

HTTP/1.1 404 Not Found
X-Powered-By: Express
Content-Type: application/json; charset=utf-8
Content-Length: 72
ETag: W/"48-JVKxxdPXsoqsb4azOatO7R7HQ28"
Date: Fri, 21 Feb 2020 09:19:07 GMT
Connection: keep-alive

{"name":"DEVICE_GROUP_NOT_FOUND","message":"Couldn\t find device group"}

The same case based in legacy expression, with the following device provision request, works as exepcted.

curl -iX POST \
  'http://localhost:4061/iot/devices' \
  -H 'Content-Type: application/json' \
  -H 'fiware-service: openiot' \
  -H 'fiware-servicepath: /' \
  -d '{
 "devices": [
   {
         "device_id": "device01",
         "entity_name": "device01",
         "entity_type": "Thing",
         "attributes":[
            {
               "name":"fillLevel",
               "type":"Number",
               "expression": "${100 * @level}"
            },
            {
               "name":"level",
               "type":"Number"
            }			
         ]
   }
 ]
}
'

@chicco785 I'd need your help to see what's going on with this :) Do you have a script or sequence of curls or similars of the "happy path case" of your implementation?

@chicco785
Copy link
Contributor Author

chicco785 commented Feb 21, 2020 via email

@fgalan
Copy link
Member

fgalan commented Feb 21, 2020

hi fermin, we have a version deployed (but not with last changes) and it's working fine :/ i am leaving now for two weeks, when i am back if you can share your image, and i will debug using that.

Probably I'm doing some step wrongly ;)

The image in telefonicaiot/iotagent-ul:jexl (which uses the library version in this PRs branch, with the latest changes) is public. In fact, if you deploy directly the docker-compose.yml I put above you will get the same setup than me. Just tell me when you get ready to start debugging.

Thanks!

@chicco785
Copy link
Contributor Author

chicco785 commented Feb 21, 2020 via email

@fgalan
Copy link
Member

fgalan commented Mar 9, 2020

@chicco785 any update on this, please? I'm very excited about the new funcionality and I'd like to start using :)

Thanks!

@chicco785
Copy link
Contributor Author

@fgalan i am just back from 2 weeks of vacations, I will try to have a look into this in the next days

@fgalan fgalan mentioned this pull request Mar 12, 2020
@fgalan
Copy link
Member

fgalan commented Apr 3, 2020

@fgalan i am just back from 2 weeks of vacations, I will try to have a look into this in the next days

Any update, please?

Last Monday FIWARE TSC (March 30th) the IOTA topics were reviewed and I wonder about the progress on this...

Thank!

@@ -156,10 +175,6 @@ function doActivate(newConfig, callback) {
], callback);
};

if (!config.getConfig().dieOnUnexpectedError) {
Copy link
Member

Choose a reason for hiding this comment

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

Why delete this check? Apparently is not related with current feature, is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was moved upfront

@AlvaroVega
Copy link
Member

I've been doing some tests using iota-ul docker image created by @fgalan and it works like expected!
I think @fgalan reported problems are not related with this issue, probably are related with other devices tests.
IMHO is LGTM but https://github.com/telefonicaid/iotagent-node-lib/pull/800/files#r420657393

if (!config.getConfig().dieOnUnexpectedError) {
process.on('uncaughtException', globalErrorHandler);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AlvaroVega not removed, but moved upfront ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i moved upfront in the code, to be sure that env variables are all checked before

@chicco785
Copy link
Contributor Author

@AlvaroVega good that you could test it, unfortunately covid screwed all my plans in relation to this PR and checks of @fgalan image. hope you are all doing good.

Copy link
Member

@AlvaroVega AlvaroVega left a comment

Choose a reason for hiding this comment

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

LGTM

@fgalan
Copy link
Member

fgalan commented May 6, 2020

@chicco785 we have done a new round of tests and it seems to work fine. Probably, I did some mistake in my test, but @AlvaroVega has found the way of making it to work :)

Thus, no need of further actions on your side regarding this PR. We are going to change the base branch to expression-lib-alternative-pre-landing to solve the CHANGES_NEXT_RELEASE conflict.

Thanks for this feature! It will help a lot in some use cases

@fgalan fgalan changed the base branch from master to expression-lib-alternative-pre-landing May 6, 2020 09:56
@fgalan fgalan merged commit 067333e into telefonicaid:expression-lib-alternative-pre-landing May 6, 2020
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.

4 participants