-
Notifications
You must be signed in to change notification settings - Fork 89
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
Provide an alternative expression plugin #800
Conversation
see #801 |
93105b2
to
545dfb3
Compare
with reference to #687 Jexl supports:
Most probably the following functions can be easily introduced:
And interestingly it allows "Object" navigation, e.g., supposing the context is
operation navigating the object are possible:
|
0981862
to
f223691
Compare
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... |
@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:
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. |
at the time being i am testing it with lorawan, and managed the flag based configuration here: the flag support could be as well moved in directly in the lib and totally removed if the grammar option works out |
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. |
@fgalan understood that, in fact i was talking about "grammar" conversion in the sense that: but as said i think could be a secondary pr, and alternative could be also a db update script. |
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. |
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. |
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:" 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. |
|
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. 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. |
I understand the extra risk about the grammar conversion approach. I thought it could empower more easily old users. What about: |
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. |
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.
Not sure if your refer to your original idea |
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)
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 |
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. |
not really, i have my dev version with it already in prod ;) i can wait
fermin!
|
I'm back :) There has been a long iteration between @mrutid and @chicco785 but I understand the final conclusion is this:
Based on that, I'll provide the following precise proposal: For item 1, the following configuration parameter:
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:
2b) Per device language setting:
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! |
doc/expressionLanguage.md
Outdated
| '" a "|trim' | 'a' | | ||
| 'name|length' | 8 | | ||
| 'name|indexOf("e")' | 1 | | ||
| 'name|substring(0,name|indexOf("e")+1)'| 'De' | |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix by 042c9fd
d829d42
to
042c9fd
Compare
JEXL is not natively supporting dates, anyhow there are some clues on how to manage that: ideally we could "test" a value to be a Date in this function: and the add a function to add a given time unit. |
There was a problem hiding this 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 :)
These are the branches in the IOTAs' repositories to test regression (the will be deleted once regression finishes): |
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 (
Now, service provision and device provision. Note the expression used (
Now I send a measure in the southbound interface (level equal to 2, expecting fillLevel equal to 200)
However what I get is this error message:
@chicco785 could you have a look, please? Maybe I'm doing something incorrectly? |
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
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):
The same case based in legacy expression, with the following device provision request, works as exepcted.
@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? |
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 Thanks! |
I don't think it is you, it's much more probable that I broke something
with the last changes :)
|
@chicco785 any update on this, please? I'm very excited about the new funcionality and I'd like to start using :) Thanks! |
@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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was moved upfront
I've been doing some tests using iota-ul docker image created by @fgalan and it works like expected! |
if (!config.getConfig().dieOnUnexpectedError) { | ||
process.on('uncaughtException', globalErrorHandler); | ||
} | ||
|
There was a problem hiding this comment.
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 ;)
There was a problem hiding this comment.
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
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@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 Thanks for this feature! It will help a lot in some use cases |
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.