Check realtime mode in remote clock interceptor #6985

shefalijoshi · 2023-08-22T18:39:55Z

Closes VIPEROMCT-397

Describe your changes:

Check that the request is using realtime before using the remote-clock start and end timestamps for telemetry requests

All Submissions:

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?
Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

Changes address original issue?
Tests included and/or updated with changes?
Command line build passes?
Has this been smoke tested?
Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

Changes appear to address issue?
Reviewer has tested changes by following the provided instructions?
Changes appear not to be breaking changes?
Appropriate automated tests included?
Code style and in-line documentation are appropriate?
Has associated issue been labelled unverified? (only applicable if this PR closes the issue)
Has associated issue been labelled bug? (only applicable if this PR is for a bug fix)

Check that the request is using realtime before using the remote-clock start and end timestamps for telemetry requests

deploysentinel · 2023-08-22T18:44:24Z

Current Playwright Test Results Summary

✅ 14 Passing - ⚠️ 1 Flaky

Run may still be in progress, this comment will be updated as current testing workflow or job completes...

(Last updated on 10/03/2023 08:40:29pm UTC)

Run Details

Running Workflow e2e-couchdb on Github Actions

Commit: 4dce3aa

Started: 10/03/2023 08:37:32pm UTC

⚠️ Flakes

📄 functional/plugins/notebook/notebookWithCouchDB.e2e.spec.js • 1 Flake

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Notebook Tests with CouchDB @couchdb Inspect Notebook Entry Network Requests Retry 1 • Initial Attempt	0% (0) 0 / 47 runs failed over last 7 days	4.26% (2) 2 / 47 runs flaked over last 7 days

View Detailed Build Results

Current Playwright Test Results Summary

✅ 141 Passing - ⚠️ 4 Flaky

Run may still be in progress, this comment will be updated as current testing workflow or job completes...

(Last updated on 10/03/2023 08:40:29pm UTC)

Run Details

Running Job e2e-stable on CircleCI

Commit: 4dce3aa

Started: 10/03/2023 08:30:33pm UTC

⚠️ Flakes

📄 functional/plugins/notebook/restrictedNotebook.e2e.spec.js • 2 Flakes

Top 1 Common Error Messages

`null`	2 Test Cases Affected

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Restricted Notebook with a page locked and with an embed @addinit Allows embeds to be deleted if page unlocked @addinit Retry 1 • Initial Attempt	7.53% (7) 7 / 93 runs failed over last 7 days	49.46% (46) 46 / 93 runs flaked over last 7 days
Restricted Notebook with a page locked and with an embed @addinit Disallows embeds to be deleted if page locked @addinit Retry 1 • Initial Attempt	0% (0) 0 / 85 runs failed over last 7 days	43.53% (37) 37 / 85 runs flaked over last 7 days

📄 functional/planning/timelist.e2e.spec.js • 1 Flake

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Time List Create a Time List, add a single Plan to it and verify all the activities are displayed with no milliseconds Retry 1 • Initial Attempt	0% (0) 0 / 132 runs failed over last 7 days	56.82% (75) 75 / 132 runs flaked over last 7 days

📄 functional/plugins/notebook/notebookSnapshots.e2e.spec.js • 1 Flake

Test Case Results

Test Case	Last 7 days Failures	Last 7 days Flakes
Snapshot image tests Can drop an image onto a notebook and create a new entry Retry 1 • Initial Attempt	17.35% (17) 17 / 98 runs failed over last 7 days	55.10% (54) 54 / 98 runs flaked over last 7 days

View Detailed Build Results

codecov · 2023-08-22T18:49:33Z

Codecov Report

Merging #6985 (4dce3aa) into master (d53d8d5) will decrease coverage by 1.62%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #6985      +/-   ##
==========================================
- Coverage   57.18%   55.57%   -1.62%     
==========================================
  Files         413      650     +237     
  Lines       12805    26102   +13297     
  Branches        0     2549    +2549     
==========================================
+ Hits         7323    14506    +7183     
- Misses       5482    10895    +5413     
- Partials        0      701     +701

Flag	Coverage Δ		*Carryforward flag
e2e-full	`41.93% <ø> (+0.01%)`	⬆️	Carriedforward from 4f1418b
e2e-stable	`57.19% <0.00%> (+2.98%)`	⬆️
unit	`49.17% <0.00%> (?)`

*This pull request uses carry forward flags. Click here to find out more.

Files	Coverage Δ
src/plugins/remoteClock/requestInterceptor.js	`18.18% <0.00%> (+18.18%)`	⬆️

... and 402 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d53d8d5...4dce3aa. Read the comment docs.

ozyx · 2023-08-22T20:28:47Z

src/plugins/remoteClock/requestInterceptor.js

-      remoteClockLoaded = true;
-      request.start = start;
-      request.end = end;
+      const timeContext = request?.timeContext ?? openmct.time;


ozyx · 2023-08-22T20:29:07Z

src/plugins/remoteClock/requestInterceptor.js

-      request.end = end;
+      const timeContext = request?.timeContext ?? openmct.time;
+      //if we're not in realtime mode, we will use the fixed time bounds provided by the request
+      if (timeContext.isRealTime()) {


Should this be if (!timeContext.isRealTime()) ?

No. We only want to take the bounds from waitForBounds() if we're in realtime mode

Can we clarify the comment? The way it reads seems confusing.

Yeah, I agree, I think this is good to go after making this comment more clear.

jvigliotta

Just one small change @ozyx pointed out.

jvigliotta · 2023-09-27T19:51:43Z

src/plugins/remoteClock/requestInterceptor.js

-      request.end = end;
+      const timeContext = request?.timeContext ?? openmct.time;
+      //if we're not in realtime mode, we will use the fixed time bounds provided by the request
+      if (timeContext.isRealTime()) {


Yeah, I agree, I think this is good to go after making this comment more clear.

akhenry · 2023-10-02T19:10:27Z

VIPEROMCT-397 was closed by this PR - akhenry/openmct-yamcs#369. Do we still need this PR?

shefalijoshi · 2023-10-02T19:41:56Z

VIPEROMCT-397 was closed by this PR - akhenry/openmct-yamcs#369. Do we still need this PR?

Yes we still need this. It was found while I was investigating that bug and you will get incorrect data if you use remote-clock.

jvigliotta

LGTM!

Revert Date.now() purge change

e339e76

Check that the request is using realtime before using the remote-clock start and end timestamps for telemetry requests

shefalijoshi requested a review from jvigliotta August 22, 2023 18:40

ozyx reviewed Aug 22, 2023

View reviewed changes

shefalijoshi requested a review from ozyx August 24, 2023 19:35

unlikelyzero added this to the Target:3.1.0 milestone Aug 24, 2023

jvigliotta requested changes Sep 27, 2023

View reviewed changes

ozyx and others added 2 commits October 3, 2023 13:19

Merge branch 'master' into fix-plots-missing-data

4f1418b

docs: clarify comment

4dce3aa

ozyx requested a review from jvigliotta October 3, 2023 20:28

jvigliotta approved these changes Oct 3, 2023

View reviewed changes

jvigliotta enabled auto-merge (squash) October 3, 2023 20:32

ozyx added the pr:e2e:couchdb npm run test:e2e:couchdb label Oct 3, 2023

github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Oct 3, 2023

jvigliotta merged commit ce59c0f into master Oct 3, 2023

jvigliotta deleted the fix-plots-missing-data branch October 3, 2023 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check realtime mode in remote clock interceptor #6985

Check realtime mode in remote clock interceptor #6985

shefalijoshi commented Aug 22, 2023 •

edited

Loading

deploysentinel bot commented Aug 22, 2023 •

edited

Loading

Test Case Results

Top 1 Common Error Messages

Test Case Results

Test Case Results

Test Case Results

codecov bot commented Aug 22, 2023 •

edited

Loading

ozyx Aug 22, 2023

ozyx Aug 22, 2023

shefalijoshi Aug 24, 2023

ozyx Sep 13, 2023

jvigliotta Sep 27, 2023

jvigliotta left a comment

jvigliotta Sep 27, 2023

akhenry commented Oct 2, 2023

shefalijoshi commented Oct 2, 2023

jvigliotta left a comment

Check realtime mode in remote clock interceptor #6985

Check realtime mode in remote clock interceptor #6985

Conversation

shefalijoshi commented Aug 22, 2023 • edited Loading

Describe your changes:

All Submissions:

Author Checklist

Reviewer Checklist

deploysentinel bot commented Aug 22, 2023 • edited Loading

Current Playwright Test Results Summary

⚠️ Flakes

Test Case Results

Current Playwright Test Results Summary

⚠️ Flakes

Top 1 Common Error Messages

Test Case Results

Test Case Results

Test Case Results

codecov bot commented Aug 22, 2023 • edited Loading

Codecov Report

ozyx Aug 22, 2023

Choose a reason for hiding this comment

ozyx Aug 22, 2023

Choose a reason for hiding this comment

shefalijoshi Aug 24, 2023

Choose a reason for hiding this comment

ozyx Sep 13, 2023

Choose a reason for hiding this comment

jvigliotta Sep 27, 2023

Choose a reason for hiding this comment

jvigliotta left a comment

Choose a reason for hiding this comment

jvigliotta Sep 27, 2023

Choose a reason for hiding this comment

akhenry commented Oct 2, 2023

shefalijoshi commented Oct 2, 2023

jvigliotta left a comment

Choose a reason for hiding this comment

shefalijoshi commented Aug 22, 2023 •

edited

Loading

deploysentinel bot commented Aug 22, 2023 •

edited

Loading

codecov bot commented Aug 22, 2023 •

edited

Loading