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

Potential for memory leaks with Telemetry Collections #5419

Closed
4 of 7 tasks
akhenry opened this issue Jun 30, 2022 · 2 comments · Fixed by #5421
Closed
4 of 7 tasks

Potential for memory leaks with Telemetry Collections #5419

akhenry opened this issue Jun 30, 2022 · 2 comments · Fixed by #5421
Labels
needs:e2e Needs an e2e test needs:test instructions Missing testing notes performance impacts or improves performance severity:blocker type:bug
Milestone

Comments

@akhenry
Copy link
Contributor

akhenry commented Jun 30, 2022

Summary

Telemetry Collections allow views to offload bounds checking to the telemetry API and focus on displaying telemetry data. Telemetry Collections will maintain historical telemetry for the duration of the currently configured time conductor bounds by design.

Although this is intended behavior, if telemetry collections are used for doing simple bounds checking in "instantaneous value" type displays, like LAD Tables and Channel Tables, it can lead to a memory leak, albeit a bounded one. These types of displays are only ever interested in the latest telemetry values, but if a TelemetryCollection is used for this purpose it will hold onto a lot of historical values unnecessarily.

Expected vs Current Behavior

A potential solution is to have Telemetry Collections inspect the request options and honor the 'latest' strategy when present. In this case it would just not retain any history.

Impact Check List

  • Data loss or misrepresented data?
  • Regression? Did this used to work or has it always been broken?
  • Is there a workaround available?
  • Does this impact a critical component?
  • Is this just a visual bug with no functional impact?
  • Does this block the execution of e2e tests?
  • Does this have an impact on Performance?
@jvigliotta
Copy link
Contributor

jvigliotta commented Jul 6, 2022

Testing

  • make sure views using Telemetry Collections are working as expected
    • LAD Tables, TelemetryView (Alphanumerics in displays), Condition Widgets, and Imagery
  • for strategy = latest views (LAD, Alphanumerics), request a LARGE timeframe (24 hrs +) in an older version of Open MCT (eg. /stable) and the new version, console.log(window.performance.memory) in both and confirm for the same settings the newer version has smaller totalJSHeapSize and usedJSHeapSize sizes
  • you can also add breakpoints in TelemetryCollections.js to check that this.boundedTelemetry always only has ONE item at most

@akhenry akhenry closed this as completed Jul 6, 2022
@unlikelyzero unlikelyzero added the needs:test instructions Missing testing notes label Jul 7, 2022
@jvigliotta jvigliotta mentioned this issue Jul 7, 2022
15 tasks
@akhenry
Copy link
Contributor Author

akhenry commented Jul 8, 2022

  • LAD Tables
    Verified fixed -
  1. Created LAD table with 11 members in system with the fix
  2. Exported to legacy system without the fix
  3. Set time window to 15 minutes in both
  4. Observed memory grow by several MB in just a few minutes in the legacy system. Memory stayed stable in the system with the fix.
  • TelemetryView Verified Fixed, confirmed that the telemetry collection was only retaining one item and that memory usage did not grow
  • Condition Widgets
    Confirmed that the TelemetryCollection only contains 1 element, even after several minutes of usage. I did notice seemingly unbounded memory growth in both current and legacy versions of the Condition Widget, however it does not appear to be due to the TelemetryCollection since it is limiting itself to 1 element, so I have filed a separate issue.
  • Imagery (regression testing) filed a separate issue for problems observed with imagery (VIPEROMCT-141). Unknown if it's related to telemetry collections or not.

@akhenry akhenry removed the unverified label Jul 8, 2022
@unlikelyzero unlikelyzero added this to To triage in Improve Test Coverage via automation Jul 17, 2022
@unlikelyzero unlikelyzero added this to Needs triage in Build 6 Blockers via automation Jul 17, 2022
@unlikelyzero unlikelyzero added this to the Sprint:2.0.7 milestone Jul 17, 2022
@unlikelyzero unlikelyzero added the needs:e2e Needs an e2e test label Jul 17, 2022
@unlikelyzero unlikelyzero moved this from Needs triage to Needs an automated test in Build 6 Blockers Jul 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:e2e Needs an e2e test needs:test instructions Missing testing notes performance impacts or improves performance severity:blocker type:bug
Projects
Build 6 Blockers
Needs an automated test
Development

Successfully merging a pull request may close this issue.

3 participants