[Conditionals] Increase performance, switch to TelemetryCollections #7841
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7841 +/- ##
==========================================
+ Coverage 57.44% 57.45% +0.01%
==========================================
Files 675 675
Lines 27325 27317 -8
Branches 2673 2673
==========================================
- Hits 15697 15696 -1
+ Misses 11292 11285 -7
Partials 336 336
... and 14 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Nice work! and nice job cleaning up that wild style code. I have a couple of suggestions where we can further improve performance, and I have some other suggestions for readability improvements.
Also this needs test(s) 😛
btw this test is failing in ci: [chrome] › functional/plugins/styling/stackedPlotStyling.e2e.spec.js:159:3 › Stacked Plot styling › styling a child object of the flexible layout properly applies that style to only that child
…sa/openmct into conditional-style-performance mergin master
|
@ozyx was this change still under review? It touches some code in a file that I'm also working on (ConditionManager.js), so I want to get it merged into my branch when it's ready |
It just needs a test. So you can probably safely rebase onto this branch and then rebase onto master once you're ready to open a PR |
…dling into the raf for consistency and performance
…sa/openmct into conditional-style-performance mergin master
| const style = await element.getAttribute('style'); | ||
|
|
||
| // eslint-disable-next-line playwright/prefer-web-first-assertions | ||
| expect(style).toBe(expectedStyle); |
There was a problem hiding this comment.
There should be a new style checks api. I didn't have time to implement it
| * @param {string} parameterName | ||
| * @param {string} objectName | ||
| */ | ||
| async function searchAndLinkParameterToObject(page, parameterName, objectName) { |
There was a problem hiding this comment.
this is a pre-promotion appAction. We're leveraging this in yamcsAppActions. I think it would be useful in core
| let stateGenerator; | ||
| let conditionSet; | ||
| let displayLayout; | ||
| const STATE_CHANGE_INTERVAL = '1'; |
There was a problem hiding this comment.
this may need to change if the current test is flaky
|
This issue was still evident in my testing - conditional.styling.bug.movI actually think that the video does not show a bug at all. I think what it shows is an artifact of telemetry batching. We send batches of telemetry once per second from the WebSocket worker, and the condition set and TelemetryView just show the most recent value from the batch. Each window is a separate WebSocket, and a separate buffer initiated at a different time, so the latest value in each batch will be different, and that's fine. We're showing a real-time "snapshot" of the system to the user once-per second. If they want to see everything they can, in a table or a plot. |
Closes #7840
Believe this may be the issue reported in: VIPERGC-550
Describe your changes:
Add some performance enhancements to conditional styling. Switched to TelemetryCollections from older and separate
subscribeand historicalrequest. This prevents situations where different telemetry was being evaluated in a condition vs the telemetry itself in the item being styled with the condition. This also just brings conditions in line with the rest of our code that uses TelemetryCollections.All Submissions:
Author Checklist
type:label? Note: this is not necessarily the same as the original issue.Reviewer Checklist