peter-murray / node-hue-api

Node.js Library for interacting with the Philips Hue Bridge and Lights
Apache License 2.0
1.18k stars 144 forks source link

Rate limiting doesn't allow max throughput of brightness commands #196

Closed philcunliffe closed 2 years ago

philcunliffe commented 3 years ago

The Hue API lists the min time between brightness commands as 40ms (https://developers.meethue.com/develop/application-design-guidance/hue-system-performance/)

It would be awesome if I could override the rate limiting via a parameter on the API creation

peter-murray commented 3 years ago

The problem is that I don't know what commands you have run and therefore the rate limiting to apply. I can just see this causing more problems as this API is possibly only one of multiple integrations that a user may have running against the Hue bridge, so I had applied the previous recommended values...

It looks like they have gotten a little more specific there with the various rate limits and calculations.

There are two different rate limiters in place inside the API today. One for groups and another for lights. I take it you are looking to modify the limiter on the lights specifically here?

If so this is the current limiter in place along with the previous recommendations provided int he API docs:

// As per Bridge documentation guidance, limit the number of calls to the light state changes to 10 per second max
this._lightStateLimiter = new Bottleneck({maxConcurrent: 1, minTime: 60});

The specific part in those docs that the above was set off is the conclusion section:

As a general guideline we always recommend to our developers to stay at roughly 10 commands per second to the /lights resource with a 100ms gap between each API call. For /groups commands you should keep to a maximum of 1 per second. It is however always recommended to take into consideration the above information and to of course stress test your app/system to find the optimal values for your application.

The means that currently whilst not at the single fastest possible speed (if all things are perfectly aligned as per the conditions they mention) of 40ms on system throughput, I have a default set to 60ms before another request will be sent. Due to the fact that 40ms is the fastest they claim there, and the next one being 80ms for brightness and color, the API is probably over sending API requests if a user goes command trigger happy...

With all the context out of the way 😉 , to satisfy your use case, would you accept some setting of fastest but possibly unstable, but damn it I know what I am doing or sensible as a configuration parameter? I am not overly fond of exposing this as a number as it means I get increased support tickets when people set these types of things to inappropriate levels in the future?

philcunliffe commented 3 years ago

Yeah I would definitely be ok with "I know what I'm doing flag" if that's your preferred method. My thought was it would just be an optional parameter when you initialize the API but either way would work for me.

Currently, I just bypass node-hue-api when I need to make non rate-limited calls which works but isn't very convenient.

peter-murray commented 3 years ago

I had to go a more "complete" route to adding the ability to tweak the rate limits as there are two that you might need to adjust in your use case. The lights and groups both have rate limits, but the underlying transport (http or https that uses node-fetch under the covers) also has another rate limiter that you may need to adjust.

This is the rate limit configuration that you would need to specify your changes/tweaks to https://github.com/peter-murray/node-hue-api/blob/typescript/src/api/HueApiRateLimits.ts

Your tweaked rate limits can then be passed into the initialization; https://github.com/peter-murray/node-hue-api/blob/typescript/src/api/index.ts#L22 of the bridge connection.

There is also a logger that you can activate to see the queuing and change that your settings may have on this https://github.com/peter-murray/node-hue-api/blob/typescript/src/api/HueApiRateLimitLogger.ts#L4.

I have released a test version 5.0.0-beta.6 if you want to try this out.

peter-murray commented 2 years ago

@philcunliffe 👋 did the above changes give you what you wanted here?

Codelica commented 2 years ago

Hello!

I think this change introduced a bug that causes the default "light" and "group" rate limits to get swapped. I was noticing I couldn't introduce light state changes faster than 1 per second by default. :)

You'll notice the swap on lines 44 and 45 in HueApiRateLimits.ts:

...
      this._rateLimits = {
        transport: DEFAULT_RATE_LIMITS,
        group: DEFAULT_LIGHT_RATE_LIMITS,
        light: DEFAULT_GROUP_RATE_LIMITS,
      }
...
peter-murray commented 2 years ago

:facepalm: Fixed in version 5.0.0-beta.11 which was just published to npm

philcunliffe commented 2 years ago

It did solve it, thanks!