TheThingsNetwork / lorawan-stack

The Things Stack, an Open Source LoRaWAN Network Server
https://www.thethingsindustries.com/stack/
Apache License 2.0
957 stars 302 forks source link

Available payload formatter size is too small #4278

Closed laurensslats closed 3 years ago

laurensslats commented 3 years ago

Summary

Available Payload formatter size is too small for some commonly used devices.

Why do we need this?

The default payload formatter's size of Elsys devices is too big for The Things Stack. Same for KLAX, Milesight and NKE devices.

The size of Elsys' payload decoder is ±1000 bytes, The Things Stack allows up to 4 kbytes. Reference: https://www.elsys.se/en/elsys-payload/

This was no issue in V2.

What is already there? What do you see now?

When Elsys' payload formatter is added, the below error is shown:

Screenshot 2021-06-16 at 16 08 03

Invalid value of field default_formatters.up_formatter_parameter

{
  "code": 3,
  "message": "error:pkg/applicationserver:field_value (invalid value of field `default_formatters.up_formatter_parameter`)",
  "details": [
    {
      "@type": "type.googleapis.com/ttn.lorawan.v3.ErrorDetails",
      "namespace": "pkg/applicationserver",
      "name": "field_value",
      "message_format": "invalid value of field `{field}`",
      "attributes": {
        "field": "default_formatters.up_formatter_parameter"
      },
      "correlation_id": "0b76c2d282e14e909081c8ec997d4778",
      "cause": {
        "namespace": "pkg/applicationserver",
        "name": "formatter_script_too_large",
        "message_format": "formatter script size exceeds maximum allowed size",
        "attributes": {
          "max_size": 4096,
          "size": 7964
        },
        "code": 3
      },
      "code": 3
    }
  ],
  "request_details": {
    "url": "/as/applications/test-application/link",
    "method": "put",
    "stack_component": "as"
  }
}

What is missing? What do you want to see?

~Increase available size to 8 or 16 kB?~

@neoaggelos see https://github.com/TheThingsNetwork/lorawan-stack/issues/4278#issuecomment-865832090

Environment

v3.13.1

How do you propose to implement this?

Get input from Jaime on the minimum required payload formatters size.

How do you propose to test this?

Upload payload formatters of >4 kbytes on test local setup.

Can you do this yourself and submit a Pull Request?

Need input form Jaime and from product team.

Jaime-Trinidad commented 3 years ago

We have different payload decoders that the console don't support because the size is over 4kb, in this cases we have Comtac, nke-watteco, IMST, digital-matter (5kb file size), Elsys. Also the console throws some errors in switch-case with variables.

IMST has two 50kb size file, and nke-watteco all their files are between 20-30 kb. Other cases are less than 16kb.

Comtac 12kb file Comtac Error with variables Comtac

imst 50 kb file Comtac

Jaime-Trinidad commented 3 years ago

we should consider size for devices that are not yet on device repository, for the moment all these including elsys are now on DR so payload formatter is already there.

johanstokking commented 3 years ago

Here's a histogram of the number of scripts per kilobyte:

Screenshot 2021-06-17 at 16 03 45

Reasons why scripts are too large:

  1. A vendor maintains one script for all their devices. As they keep adding devices with new sensors, data types, status codes, commands etc, the one script keeps increasing
  2. The vendor includes all sorts of boilerplate for decoding all integer and floating point lengths, signed/unsigned, both byte orders, etc, or even polyfills for Node's Buffer class
  3. Lots of comments, tables
  4. Indentation with spaces instead of tabs

Now, these are actually all valid reasons, and there is little we can do about this. Sure, we can include a non-standard library of useful functions, or obfuscate the code that drastically reduces the file size, but that doesn't make the experience any better.

Currently, there's a LoRa Alliance task force under TC about Codec API where a limit is being defined. I think we'll end up somewhere in the range of 32 to 40 KB.

Therefore, I think we should consider increasing the limit, also for scripts that are stored per-device AS. Rather, we should think about optimizing storage of AS:

descartes commented 3 years ago

Regardless of providing some more space and deduplication, some effort should be expected by the vendors to tame their formatters - huge monolithic blocks of JS does nothing to help the end user with optimisation & alterations.

As an interim measure, perhaps increasing the limit on the console so that perhaps a code library could be included. In the meanwhile, I'm in the process of filing a rounding error!

adriansmares commented 3 years ago

The original reason for which we actually limited the formatted size was not about storage problems on our side, but rather SLA: parsing huge scripts slows down the uplink pipeline and leads to formatters not being run - users do not understand this, and to be honest they shouldn't - this is mostly an implementation detail.

In my opinion, we should do the following:

johanstokking commented 3 years ago
  • Increase API limit to 40 KB
  • Limit in the configuration the value to 32 KB

Let's do this for 3.13.3.

I'd prefer caching the ASTs (by hash) as part of https://github.com/TheThingsIndustries/lorawan-stack/issues/2264.

descartes commented 3 years ago

storage problems on our side

My understanding was that it filled up Redis as well as impacting on server resources

users do not understand this, and to be honest they shouldn't - this is mostly an implementation detail.

Why shouldn't they - otherwise they'll just eat all the resources you put in to it - cars have resource limits. so this isn't an alien concept.

  • Maybe minify the device repository payload formatters ?

If you keep the original formatter compressed in slower storage for when they are going to be edited, why not minify all of them for the Redis store?

  • Optimize the storage using deduplication.

I've looked at how we can attach a device registration to a repository device so it can pickup the payload formatter for some forum users - so there are people around currently copying & pasting formatters that could be picked up from the repository - or as you say, de-duplicated if they are identical

adriansmares commented 3 years ago

Why shouldn't they - otherwise they'll just eat all the resources you put in to it - cars have resource limits. so this isn't an alien concept.

The limit should exist, it's just that it shouldn't be trivial to hit it, especially with already existing scripts. That's what I wanted to say in my message (but worded it poorly).

If you keep the original formatter compressed in slower storage for when they are going to be edited, why not minify all of them for the Redis store?

My caching proposal refers to this, but instead of keeping the minified script in Redis, we keep the parsed script in the memory of the Application Server. Deduplication then handles the actual storage space minimization in the Redis storage.