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

feat: Diagnostics panel #793

Merged
merged 54 commits into from
Aug 13, 2022
Merged

feat: Diagnostics panel #793

merged 54 commits into from
Aug 13, 2022

Conversation

matmen
Copy link
Member

@matmen matmen commented Jul 26, 2022

Adds a new opt-in diagnostics panel with customizable metrics logging.

Closes #454

This is needed in preparation for user-defined cards in layouts, as the default state cannot reflect those and the old method would would simply replace them with the default state.
Tested to work with added components and removed components.

Signed-off-by: Mathis Mensing <[email protected]>
Signed-off-by: Mathis Mensing <[email protected]>
@matmen matmen added the FR - Enhancement New feature or request label Jul 26, 2022
todo: modify config modals

Signed-off-by: Mathis Mensing <[email protected]>
Signed-off-by: Mathis Mensing <[email protected]>
@matmen
Copy link
Member Author

matmen commented Jul 27, 2022

I have this working to a point where data is automatically collected and charts will dynamically be rendered.
The only thing missing now is a proper, working layout for the configuration modals, but I'm not sure what the best way would be to implement those.
Not sure if the current data structure is the most optimal yet or if there's other info missing (for example for dynamic computation based on properties? e.g. converting live_extruder_velocity to flow? This would be much more complex, I'm not sure how to implement this UI wise and also code wise. Suggestions welcome 😉)

image

@matmen
Copy link
Member Author

matmen commented Jul 27, 2022

Adding the ability to script metrics would be very handy (think

Math.PI * (printer.printer.extruder.filament_diameter / 2) ** 2 * printer.printer.motion_report.live_extruder_velocity

to calculate volumetric flow), but this would require some sort of sandbox or custom parsing..

The alternative would be creating a fixed list of loggable metrics, but this kinda locks this feature down a lot.

@pedrolamas
Copy link
Member

Adding the ability to script metrics would be very handy

That would be really neat!! I guess the "hammer" approach would be to use eval and that is something we DO NOT want to do!

@matmen
Copy link
Member Author

matmen commented Jul 28, 2022

I guess the "hammer" approach would be to use eval and that is something we DO NOT want to do!

Correct. I guess the best approach here would be injecting a sandboxed iframe which would then evaluate the code in an isolated context. Should be fairly easy to implement tbh, I'll work on that later.

matmen added 12 commits July 28, 2022 20:20
Signed-off-by: Mathis Mensing <[email protected]>
Signed-off-by: Mathis Mensing <[email protected]>
Signed-off-by: Mathis Mensing <[email protected]>
This saves a lot of performance, and I don't think anything outside of the printer state is required for logging.

Signed-off-by: Mathis Mensing <[email protected]>
@pedrolamas
Copy link
Member

Really, REALLY love this new feature!! I think this will make a lot of users very happy!!

I'm not sure of these buttons:

image

Either change to round ones, or instead of icons just add text to them (either way matching what we do on the other dialogs)

"Add Card" is correct, but I think "Add Chart" would make more sense (these are all charts, correct?)

@pedrolamas
Copy link
Member

Not sure of the impact of enabling diagnostics, but should we add a footer here with a warning?

image

Signed-off-by: Mathis Mensing <[email protected]>
@matmen
Copy link
Member Author

matmen commented Aug 5, 2022

Not sure of the impact of enabling diagnostics, but should we add a footer here with a warning?

The impact isn't negligible (about a 5.7% decrease in idle time from what I've measured).
What do you think the wording here should be, Logging diagnostics info may impact performance?

Signed-off-by: Mathis Mensing <[email protected]>
@matmen
Copy link
Member Author

matmen commented Aug 5, 2022

I did notice that during printing, the 250ms update rate makes the configuration dialog pretty much unusable (with the charts being re-rendered every time).

Edit: Also looks like the entire UI becomes unresponsive after a couple minutes, will definitely need to check that out...

@pedrolamas
Copy link
Member

What do you think the wording here should be, Logging diagnostics info may impact performance?

Works for me! 🙂

reduces the call time of onDiagnosticsMetricsUpdate by ~43%

Signed-off-by: Mathis Mensing <[email protected]>
reduces the call time of onDiagnosticsMetricsUpdate by another ~59%

Signed-off-by: Mathis Mensing <[email protected]>
@matmen
Copy link
Member Author

matmen commented Aug 6, 2022

I've optimized the metrics collection a bit further (from ~20ms to ~5ms): image

I'm still unsure why the client starts freezing after a while, from performance monitoring it looks like everything just gets slower (proportionally the same), even things unrelated to this feature. I assume it has something to do with the state being polluted, but I'm not 100% sure.

@matmen
Copy link
Member Author

matmen commented Aug 6, 2022

Well, looks like it indeed is "polluting" the state, I didn't think storing the metrics in the state would make this much difference.. Basically, with a retention of 10 minutes, there's up to 3000 data points in the state during printing (compared to 600 max per other chart). Not sure how to solve this..

@matmen matmen added the Docs - Needed Docs need to be added for tagged issue label Aug 7, 2022
@matmen
Copy link
Member Author

matmen commented Aug 7, 2022

I've stumbled across this thread on the Vue forum which describes a similar problem, and it looks like the performance issues aren't there in the production build (we disable strict mode in production builds).
Not sure what the implications of disabling strict mode are in the dev build, so I would just leave it enabled and cope with the performance loss there.
Below are the two CPU load graphs, upper one is the production build, lower one the dev build.

image

@pedrolamas
Copy link
Member

I will try to take a look into this too (later today if I manage, or tomorrow), but I'm starting to wonder if this shouldn't actually be done on Moonraker (offload to the server)

src/plugins/sandboxedEval.ts Show resolved Hide resolved
src/components/common/CollapsableCard.vue Show resolved Hide resolved
src/store/printer/actions.ts Show resolved Hide resolved
@matmen matmen merged commit f61eaf4 into fluidd-core:develop Aug 13, 2022
@matmen matmen deleted the feat/diagnostics branch August 13, 2022 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs - Needed Docs need to be added for tagged issue FR - Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Abilty to graph additional metrics, specificly flow rate and speed.
2 participants