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

Better device proxy cache #4792

Merged
merged 10 commits into from
Dec 18, 2024
Merged

Better device proxy cache #4792

merged 10 commits into from
Dec 18, 2024

Conversation

hardillb
Copy link
Contributor

@hardillb hardillb commented Nov 18, 2024

part of #3414

Description

Rather than requesting the 4 largest files from the device agent every time the device editor is loaded it will load them from a local instance on @node-red/editor-client of the matching version if present.

Still need to work out how to install the right versions of @node-red/editor

Related Issue(s)

#3414

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • Includes a DB migration? -> add the area:migration label

Copy link

codecov bot commented Nov 18, 2024

Codecov Report

Attention: Patch coverage is 54.83871% with 14 lines in your changes missing coverage. Please review.

Project coverage is 78.59%. Comparing base (33ac8f7) to head (3b83122).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
forge/ee/lib/deviceEditor/DeviceTunnelManager.js 45.45% 12 Missing ⚠️
forge/db/controllers/Device.js 50.00% 1 Missing ⚠️
...db/migrations/20241218-01-add-nr-version-device.js 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4792      +/-   ##
==========================================
- Coverage   78.64%   78.59%   -0.05%     
==========================================
  Files         324      325       +1     
  Lines       15265    15296      +31     
  Branches     3507     3512       +5     
==========================================
+ Hits        12005    12022      +17     
- Misses       3260     3274      +14     
Flag Coverage Δ
backend 78.59% <54.83%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Pervious code had hard coded path on my machine, this moves it to
under FLOWFORGE_HOME in `var/device/cache`
@hardillb hardillb requested a review from Steve-Mcl December 17, 2024 16:58
@hardillb hardillb marked this pull request as ready for review December 17, 2024 16:58
@hardillb
Copy link
Contributor Author

hardillb commented Dec 17, 2024

@Steve-Mcl the default path for a localfs test is var/device/cache in the flowfuse directory (same place as var/stacks and var/projects for localfs)

And @node-red/editor-client needs to be installed under that in the matching Node-RED version number (like stacks)

e.g. in var/device/cache create 4.0.5 and run npm install @node-red/editor-client@4.0.5 --prefix 4.0.5

Also needs the head of device agent to work

@Steve-Mcl
Copy link
Contributor

@Steve-Mcl the default path for a localfs test is var/device/cache in the flowfuse directory (same place as var/stacks and var/projects for localfs)

And @node-red/editor-client needs to be installed under that in the matching Node-RED version number (like stacks)

e.g. in var/device/cache create 4.0.5 and run npm install @node-red/editor-client@4.0.5 --prefix 4.0.5`

Also needs the head of device agent to work

Thanks Ben. First job for me this morning will be to pull and test this locally.

Copy link
Contributor

@Steve-Mcl Steve-Mcl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ben, this review is next on my list but before I pull and test, can you update the date on the migration please - there have been other migrations since Nov 15th.

Thanks.

@hardillb
Copy link
Contributor Author

@Steve-Mcl done (this will probably break the pre-staging env)

@Steve-Mcl
Copy link
Contributor

Steve-Mcl commented Dec 18, 2024

@Steve-Mcl done (this will probably break the pre-staging env)

@ppawlowski is it possible to delete / refresh the pre-staging for this?

@hardillb will this affect staging?

@hardillb
Copy link
Contributor Author

@Steve-Mcl we have already deleted the pre-staging env, we did not recreate it. It can't be tested on pre-staging as there is no broker for the device comms needed to enable device editor mode.

At the moment we are not deploying the @node-red/editor-client packages in the containers so this will run as "normal" and not use the cached copies. The update to the containers will get done later.

Copy link
Contributor

@Steve-Mcl Steve-Mcl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in slack, I added the try-catch and fall through to original tunneling logic.

This works fantastically well Ben I gotta say, its like night & day!

For the record, I added temporary logging to ensure things were working as expected, here are the results:

Serving cached file: C:\Users\xxx\repos\github\flowfuse\dev-env\packages\flowfuse\var\device\cache\4.0.7\node_modules\@node-red\editor-client\public\red\style.min.css
Serving cached file: C:\Users\xxx\repos\github\flowfuse\dev-env\packages\flowfuse\var\device\cache\4.0.7\node_modules\@node-red\editor-client\public\vendor\vendor.js
Serving cached file: C:\Users\xxx\repos\github\flowfuse\dev-env\packages\flowfuse\var\device\cache\4.0.7\node_modules\@node-red\editor-client\public\red\red.min.js
Serving cached file: C:\Users\xxx\repos\github\flowfuse\dev-env\packages\flowfuse\var\device\cache\4.0.7\node_modules\@node-red\editor-client\public\vendor\monaco\dist\editor.js
Serving cached file: C:\Users\xxx\repos\github\flowfuse\dev-env\packages\flowfuse\var\device\cache\4.0.7\node_modules\@node-red\editor-client\public\red\style.min.css
Serving cached file: C:\Users\xxx\repos\github\flowfuse\dev-env\packages\flowfuse\var\device\cache\4.0.7\node_modules\@node-red\editor-client\public\vendor\vendor.js
Serving cached file: C:\Users\xxx\repos\github\flowfuse\dev-env\packages\flowfuse\var\device\cache\4.0.7\node_modules\@node-red\editor-client\public\red\red.min.js
Serving cached file: C:\Users\xxx\repos\github\flowfuse\dev-env\packages\flowfuse\var\device\cache\4.0.7\node_modules\@node-red\editor-client\public\vendor\monaco\dist\editor.js
[2024-12-18T12:20:59.777Z] INFO: Device 36LO0L3Odb tunnel id:1 - new editor connection req:54 
Serving cached file: C:\Users\xxx\repos\github\flowfuse\dev-env\packages\flowfuse\var\device\cache\4.0.7\node_modules\@node-red\editor-client\public\vendor\mermaid\mermaid.min.js
Serving cached file: C:\Users\xxx\repos\github\flowfuse\dev-env\packages\flowfuse\var\device\cache\4.0.7\node_modules\@node-red\editor-client\public\vendor\monaco\dist\css.worker.js
Serving cached file: C:\Users\xxx\repos\github\flowfuse\dev-env\packages\flowfuse\var\device\cache\4.0.7\node_modules\@node-red\editor-client\public\vendor\monaco\dist\ts.worker.js

@hardillb hardillb merged commit 0eb0082 into main Dec 18, 2024
17 of 18 checks passed
@hardillb hardillb deleted the better-device-proxy-cache branch December 18, 2024 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants