search

dxdc / homebridge-blinds

:sunrise: Homebridge Plugin to control my blinds over HTTP
https://www.npmjs.com/package/homebridge-blinds
ISC License
54 stars 26 forks source link

Adding timing (ms) to httpRequest #33

Closed SupImDos closed 4 years ago

SupImDos commented 4 years ago

Hi,

I have been wanting to check how long the setCurrentPositionByUrl() http requests have been taking while in verbose mode, to check that the ESP32 which controls my blinds is responding in a timely fashion to the GET requests.

This timing is similarly as useful as the timing done on a move command (setTargetPosition()).

If you feel it would be better to time using a Date.now() - startTimestamp method (as per setTargetPosition()), then we could go that route as well, to keep things consistent. However, I had read online that the response.elapsedTime method is a better way to time a http request due to its asynchronous nature (citation needed???).

Thanks!

SupImDos commented 4 years ago

Also, I have no sentimental attachment to this pull request, I just didn't want to request a feature without implementing it myself first, would have felt bad.

If you feel there is a better way to do the timing, or a better place to time (i.e. in the actual setCurrentPositionByUrl() function instead of the httpRequest() function) then I am happy to implement it that way.

Thanks!

dxdc commented 4 years ago

@SupImDos Thanks for the PR. Because you mentioned you don't have a sentimental attachment to it, I went ahead and implemented it a bit differently, see: https://github.com/dxdc/homebridge-blinds/commit/fd2022da595cf0cf5110fcdcabb8bd41b5af627e

  1. Instead of always using the time option, a user can just set it in their config.json as part of their up_url, down_url, etc. This allows more flexibility for monitoring specific requests only.

  2. I refactored httpRequest a bit to allow requestTime as a returned parameter. It's only used in one place right now, but can be included in different locations later if desired.

  3. elapsedTime is deprecated, and also, only includes the 'download' portion of the timing (e.g., not dns lookup, etc.). Therefore, I used timingPhases instead, which appears to have a lot more detail. If time is used in the request object, it will log these parameters.

To test these changes, please try:

SupImDos commented 4 years ago

@dxdc

Perfect! Makes much more sense this way, thanks very much for the changes. I'll close this pull request now.