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

Reduce payload formatter script size #4053

Closed
johanstokking opened this issue Apr 13, 2021 · 2 comments
Closed

Reduce payload formatter script size #4053

johanstokking opened this issue Apr 13, 2021 · 2 comments
Assignees
Labels
c/application server This is related to the Application Server in progress We're working on it scalability This could become a problem at scale
Milestone

Comments

@johanstokking
Copy link
Member

Summary

Replaces https://github.com/TheThingsIndustries/lorawan-stack/issues/2675

Reduce the maximum script size for payload formats to 4 KB.

Why do we need this?

Because it takes too much memory otherwise.

We will allow larger scripts via the Device Repository. We have optimizations for that planned, see https://github.com/TheThingsIndustries/lorawan-stack/issues/2264.

What is already there? What do you see now?

No limits. We already see scripts larger than 200 KB.

What is missing? What do you want to see?

Limits

How do you propose to implement this?

This applies to user defined scripts on the application and device level. This does not affect the message processor parameter field (for now), as we still allow larger values from device repository.

Setting this to a new minor as this is a somewhat breaking behavioral API change for users. Some scripts are for example already stored, and they cannot be updated or changed, and new devices and applications cannot be assigned a script that existing devices or applications are using.

How do you propose to test this?

Local testing

Can you do this yourself and submit a Pull Request?

Can review

@johanstokking johanstokking added c/application server This is related to the Application Server scalability This could become a problem at scale labels Apr 13, 2021
@johanstokking johanstokking added this to the v3.13.0 milestone Apr 13, 2021
@github-actions github-actions bot added the needs/triage We still need to triage this label Apr 13, 2021
@nejraselimovic nejraselimovic removed the needs/triage We still need to triage this label Apr 19, 2021
@descartes
Copy link

On v2 when the servers were under extreme pressure the JS decoder would time out so the HTTP Integration, MQTT & Data Storage only had the raw payload - didn't bother me, I use the JS decoder for debugging only - but it did tend to cause a stir for the TTN users.

It would be foolish to say that this could never happen on v3, and 4KB is more than plenty of code which will keep processing times down, but is it worth having a status field so that if decoding does time out or otherwise fail, that it's clear in the relayed JSON that decoded_payload is going to be empty?

@johanstokking
Copy link
Member Author

@descartes good suggestion to include this somehow in the payload, potentially with the already existing errors field of decoded_payload.

We actually see V3 also already failing on large scripts. We measure the 100 ms execution timeout to include the evaluation time of the script, so with large scripts and CPU pressure, that fails just like in V2. Although we use a different JavaScript engine, this is a very similar design in V3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c/application server This is related to the Application Server in progress We're working on it scalability This could become a problem at scale
Projects
None yet
Development

No branches or pull requests

4 participants