-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Support telemetry batching and move WebSocket handling to worker #7391
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #7391 +/- ##
==========================================
- Coverage 55.82% 55.52% -0.31%
==========================================
Files 666 668 +2
Lines 26514 26701 +187
Branches 2585 2585
==========================================
+ Hits 14801 14825 +24
- Misses 10998 11160 +162
- Partials 715 716 +1
*This pull request uses carry forward flags. Click here to find out more.
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @akhenry! I'm putting a general code review here, but will check this code against YAMCS in the other PR for more thorough testing. I had a few questions below.
Review comments addressed, just doing some regression testing... |
Done, ready for re-review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested again and works great!
Blocked on legit test failure |
Closes #7390
Describe your changes:
strategy
when subscribing to telemetry. The supported strategies are:latest
- This is the default strategy. Thelatest
strategy will always return the most recent value for a given telemetry point. Depending upon the implementation details of the telemetry provider, for performance reasons a telemetry provider may drop intermediate telemetry values in order to prioritize the latest value when using thelatest
strategy. This is appropriate for Alphanumeric views, LAD Tables, etc.batch
- Thebatch
strategy invokes subscription callbacks with an array containing all of the telemetry values received since the last batch. This strategy is used by plots and tables, where intermediate values matter and should not typically be discarded.Note on the worker implementation
The worker implementation uses ES6 classes to organize the code, but because of the method used to inline the worker all of the classes have to be in the same file. I am open to suggestions on how to inline workers in a cleaner way.
Note on backwards compatibility
The changes in this PR are intended to be entirely backwards compatible. The BatchingWebSocket and the new batching subscribe strategy are opt-in. Any incompatibilities discovered should be treated as bugs.
All Submissions:
Author Checklist
type:
label? Note: this is not necessarily the same as the original issue.Reviewer Checklist