Request batch when idle #7526
Request batch when idle #7526
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7526 +/- ##
==========================================
- Coverage 55.64% 55.29% -0.35%
==========================================
Files 672 672
Lines 26996 27060 +64
Branches 2626 2626
==========================================
- Hits 15023 14964 -59
- Misses 11254 11373 +119
- Partials 719 723 +4
... and 10 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| @@ -245,7 +245,6 @@ export default function installWorker() { | |||
| } | |||
There was a problem hiding this comment.
Will try and figure out how to get Webpack to inline Workers so we can do imports etc. Should not block merge right now, it's a no-risk maintenance change we can make later.
There was a problem hiding this comment.
Some discussion on this here with some potential options - webpack/webpack#14066
There was a problem hiding this comment.
This could happen post-merge as it's low risk. It either loads or doesn't.
| @@ -352,6 +364,33 @@ export default function installWorker() { | |||
| } | |||
| } | |||
|
|
|||
| function throttle(callback, wait) { | |||
There was a problem hiding this comment.
Needed because can't import in workers with current build config.
| @@ -33,6 +33,6 @@ export default class OpenInNewTab { | |||
| } | |||
| invoke(objectPath, urlParams = undefined) { | |||
| let url = objectPathToUrl(this._openmct, objectPath, urlParams); | |||
| window.open(url); | |||
| window.open(url, undefined, "noopener"); | |||
There was a problem hiding this comment.
noopener means that the new window/tab does not share the same event loop as the opening window. It makes debugging A LOT easier and probably gives a subtle performance boost.
There was a problem hiding this comment.
| @@ -498,11 +498,17 @@ export default { | |||
| }, | |||
| created() { | |||
| this.filterTelemetry = _.debounce(this.filterTelemetry, 500); | |||
| const throttledFunc = _.throttle( | |||
There was a problem hiding this comment.
Call throttle callback on leading-edge so that screen update happens in same event loop iteration as telemetry receipt. Improves debugging and also means telemetry is not delayed an additional second beyond the delay created by telemetry batching.
| }, | ||
| mounted() { | ||
| this.csvExporter = new CSVExporter(); | ||
| this.rowsAdded = _.throttle(this.rowsAdded, 200); |
There was a problem hiding this comment.
Throttling updateVisibleRows now, no need to also throttle rowsAdded and rowsRemoved
…metry receipt so we're not adding additional delay
There was a problem hiding this comment.
This all looks good.
During testing I found one issue - after websocket is disconnected (simulated by killing quickstart locally) and reconnects after a try or two, the remote clock doesn't tick anymore. I don't think this is related to the batching changes though?
Good catch, thanks, lemme take a look. |
This was a really good catch. There was some functionality missing to re-subscribe after a WebSocket drops, which was a bad miss on my part. I've fixed the issue and added a test to catch a regression here. |
|
@shefalijoshi Addressed issue with |
Closes akhenry/openmct-yamcs#424
Related branches
Describe your changes:
All Submissions:
Author Checklist
type:label? Note: this is not necessarily the same as the original issue.Reviewer Checklist