Multi-Stream log component #4845

sahil143 · 2020-03-27T21:54:35Z

Fixes: https://issues.redhat.com/browse/ODC-2381
https://issues.redhat.com/browse/ODC-2408

Analysis / Root cause:

Create a Multi Stream Log component

Solution Description:

This PR adds a Multi Stream Log component

Screen shots / Gifs for design review:

UPDATED SCREENSHOT

Unit test coverage report:

Test setup:

Browser conformance:

Chrome
Firefox
Safari
Edge

andrewballantyne

@sahil143 Definitely a few more things needed.

I'll test around and see if I see anything else -- this was mainly the review of the code.

/assign

frontend/packages/console-shared/src/co-fetch/co-fetch.ts

frontend/packages/dev-console/src/components/pipelineruns/logs/Logs.scss

frontend/packages/dev-console/src/components/pipelineruns/detail-page-tabs/PipelineRunLogs.scss

frontend/packages/dev-console/src/components/pipelineruns/logs/Logs.tsx

frontend/packages/dev-console/src/components/pipelineruns/logs/MultiStreamLogs.scss

frontend/packages/dev-console/src/utils/__tests__/pipeline-utils.spec.ts

frontend/public/components/utils/resource-log.jsx

frontend/packages/console-shared/src/hooks/screenfull.ts

frontend/packages/dev-console/src/components/pipelineruns/logs/Logs.scss

frontend/packages/dev-console/src/components/pipelineruns/logs/Logs.tsx

frontend/packages/dev-console/src/components/pipelineruns/logs/MultiStreamLogs.tsx

frontend/packages/dev-console/src/components/pipelineruns/logs/logs-utils.ts

andrewballantyne · 2020-03-30T21:20:06Z

frontend/public/co-fetch.js

@@ -124,8 +124,8 @@ export const coFetchUtils = {
  parseJson,
 };

-export const coFetchJSON = (url, method = 'GET', options = {}) => {
-  const headers = { Accept: 'application/json' };
+export const coFetchCommon = (url, method = 'GET', options = {}, customHeaders = {}) => {


FYI @benjaminapetersen @spadgett

You guys cool with this change? I think it's helpful just to merge the fetch logic together and allow for a returning of plain text for the log component to make use of a static download (vs from a websocket).

@sahil143 Perhaps reach out to them on Slack and link them to this change in your PR (or this comment). We should get buy-in from the Admin team.

cc @christianvogt

...end/packages/dev-console/src/components/pipelineruns/logs/__tests__/MultiStreamLogs.spec.tsx

frontend/packages/console-shared/src/index.ts

frontend/packages/dev-console/src/components/pipelineruns/logs/Logs.tsx

andrewballantyne · 2020-03-31T14:25:04Z

frontend/packages/dev-console/src/components/pipelineruns/logs/MultiStreamLogs.tsx

+  React.useEffect(() => {
+    if (!scrollPane.current) return;
+    const height = Math.floor(
+      window.innerHeight - scrollPane.current.getBoundingClientRect().top - (isFullScreen ? 0 : 50),


Yeah, can we put this as padding on the top div under the classname of just odc-multi-stream-logs? I think this is a better way to go about it than a random 50 here.

andrewballantyne · 2020-03-31T14:25:45Z

frontend/packages/dev-console/src/components/pipelineruns/logs/MultiStreamLogs.tsx

+    if (!scrollPane.current) return;
+    let logString = scrollPane.current.innerText;
+    // Removing 'Taskname' from file content and using it in 'filename' to keep it similar with resource-log
+    logString = logString.substring(logString.indexOf('\n') + 1);


If you do not have a solution for the auto scroll problem, we can address it in another PR... let's not hold this one up indefinitely as we figure out that issue.

andrewballantyne · 2020-03-31T16:51:23Z

/kind feature

frontend/packages/console-shared/src/hooks/scroll.ts

frontend/packages/dev-console/src/components/pipelineruns/logs/Logs.tsx

frontend/packages/console-shared/src/hooks/scroll.ts

frontend/packages/dev-console/src/components/pipelineruns/logs/Logs.tsx

andrewballantyne

Soooooo close, just saw an error as I was about to lgtm this.

Error was an object and React crashed. Came through the websocket on the first task of a nodejs Pipeline (from the add flow).

frontend/packages/console-shared/src/hooks/scroll.ts

frontend/packages/dev-console/src/components/pipelineruns/logs/Logs.tsx

andrewballantyne

Wrong [review] type lol

andrewballantyne

/lgtm

christianvogt · 2020-04-08T19:42:50Z

I'm still seeing logs stream in the wrong order for a completed task with several steps:

Here's a screenshot where you can see the 2nd step has displayed before the first.

sahil143 · 2020-04-09T18:46:24Z

I'm still seeing logs stream in the wrong order for a completed task with several steps:
Here's a screenshot where you can see the 2nd step has displayed before the first.

@christianvogt fixed. Please review it again.

andrewballantyne · 2020-04-09T18:46:21Z

frontend/packages/dev-console/src/components/pipelineruns/logs/Logs.tsx

+        .onclose(() => {
+          onCompleteRef.current(name);
+        })
+        .onerror(() => {


Something is fishy here... I tried to find the cause but I'm a bit perplexed. On the first step of each task, it errors out with a 400 (client) error. The websocket logs are entirely unhelpful and the error is the websocket is closed (readyState === 3).

Pods do not have this issue 😕 If I go to the Pod fast enough, I see it log out without an issue and then close the websocket. Not sure what we are doing differently here.

We should definitely track this down, but I think we can leave this for now and swing back in 4.5 bug fixing. This is super odd and we are clearly missing something here. Please log a bug to track this @sahil143

Also... Console does a retry shindig if they showed an error:

<Alert isInline className="co-alert" variant="danger" title="An error occurred while retrieving the requested logs." action={<AlertActionLink onClick={this._restartStream}>Retry</AlertActionLink>} />

@andrewballantyne I only got this when Websocket request timed out with an error

WebSocket connection to 'ws://localhost:9000/api/kubernetes/api/v1/namespaces/t0s/pods/nodejs-ex-git-a01t7k-build-7d64p-pod-6t9j2/log?container=step-create-dir-image-p8tlw&follow=true' failed: Error during WebSocket handshake: Unexpected response code: 400

Otherwise I'm getting proper logs

Right, and it happened for me every time (tested it at least half a dozen times in a row) for that first step while it was being fetched by the websocket. If you come to this log/task after that first one is done, I see no error.

Still don't see this on the Pod log, clearly something is different. Since it's not critical and we have some bandwidth next sprint, let's just log something to investigate.

Raised a Jira ticket to track this https://issues.redhat.com/browse/ODC-3540.

andrewballantyne · 2020-04-09T18:47:47Z

frontend/packages/dev-console/src/components/pipelineruns/logs/LogsWrapperComponent.tsx

+  if (!_.isEmpty(obj?.data)) {
+    ref.current = obj.data;
+  }
+  return obj ? <MultiStreamLogs taskName={taskName} resource={ref.current} /> : null;


Not that it matters... but why not

Suggested change

return obj ? <MultiStreamLogs taskName={taskName} resource={ref.current} /> : null;

return ref.current ? <MultiStreamLogs taskName={taskName} resource={ref.current} /> : null;

Odd to put a catch around obj but ignore the catch logic when you go to rendering.

andrewballantyne · 2020-04-09T18:49:04Z

frontend/packages/dev-console/src/components/pipelineruns/logs/MultiStreamLogs.tsx

+      />
+      <div
+        className={classNames('odc-multi-stream-logs__container', {
+          'is-edge': window.navigator.userAgent.includes('Edge'),


Remove, not used anymore.

andrewballantyne · 2020-04-09T18:49:51Z

frontend/packages/dev-console/src/components/pipelineruns/logs/MultiStreamLogs.tsx

+        onScroll={handleScrollCallback}
+        data-test-id="logs-task-container"
+      >
+        <div className="odc-multi-stream-logs__container--pos">


-- is a modifier... you're not modifying this one, this would just be an element, no?

andrewballantyne · 2020-04-09T18:51:01Z

frontend/packages/dev-console/src/components/pipelineruns/logs/MultiStreamLogs.tsx

+          <div className="odc-multi-stream-logs__taskName">
+            {taskName}
+            {stillFetching && (
+              <span className="odc-multi-stream-logs__taskName--loading">


Same here, don't think the modifier is needed as this is just a conditional element.

Suggested change

<span className="odc-multi-stream-logs__taskName--loading">

<span className="odc-multi-stream-logs__loadingIndicator">

(for instance)

andrewballantyne · 2020-04-09T18:54:21Z

frontend/report.20200409.022812.282598.0.001.json

@@ -0,0 +1,612 @@
+


What is this? Some debugging report?

yes, these are created when webpack build goes out of memory, My bad didn't notice this.

andrewballantyne

Oh, meant to add this to my review... I'll have to retest after the latest changes... but Edge expands fine now with the fullscreen changes you did to support modifiers.

andrewballantyne · 2020-04-09T19:27:17Z

@sahil143 can you squash your commits... I only see maybe 2 distinct items in there... The coFetchText one and this component refactor. So please make it into 1/2 commits (whichever you prefer)

sahil143 · 2020-04-09T19:38:24Z

@sahil143 can you squash your commits... I only see maybe 2 distinct items in there... The coFetchText one and this component refactor. So please make it into 1/2 commits (whichever you prefer)

squashed commit into one

openshift-cherrypick-robot · 2020-04-09T20:04:32Z

@andrewballantyne: once the present PR merges, I will cherry-pick it on top of release-4.4 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.4

Need to aim to get this into 4.4 before GA as not to have a broken 4.4 feature the release it was added 😄

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

andrewballantyne · 2020-04-09T20:05:08Z

Ah shoot! Wrong PR! haha...

/cherry-pick cancel

openshift-cherrypick-robot · 2020-04-09T20:05:09Z

@andrewballantyne: once the present PR merges, I will cherry-pick it on top of cancel in a new PR and assign it to you.

In response to this:

Ah shoot! Wrong PR! haha...

/cherry-pick cancel

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

commit add http request a make reuested changes add vars fix lint errors add unit tests remove comments remove co-fetch code remove co fetch reference make request changes make requested changes make requested changes changes make requested changes rmeove fragment add tests make changes create coFetchText utility remove Accept Header use flex to determine the height of logs container, fix useScrollDirection and useFullscreen, fixe logs.ts to use ref for callbacks fix unit test change fullscreen hook implementation to use native fullscreen api instead of screenfull package full screenD remove data-id add null check for fullscreen fix overflow for logs make requested changes support older browser using prefixes make requesteddd changes changes small fix fix order for steps fix unit tests make requested changes add z-index make requested changes changes

andrewballantyne

/lgtm

This is definitely in a better place now. Thanks for the changes @sahil143

There are still a few QoL things I think we can chase down to make this experience even better. But let's put that off to another PR and likely after Feature Freeze.

christianvogt · 2020-04-14T20:54:14Z

/approve

openshift-ci-robot · 2020-04-14T20:54:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, christianvogt, sahil143

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [christianvogt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-cherrypick-robot · 2020-04-14T22:33:39Z

@andrewballantyne: cannot checkout cancel: error checking out cancel: exit status 1. output: error: pathspec 'cancel' did not match any file(s) known to git.

In response to this:

Ah shoot! Wrong PR! haha...

/cherry-pick cancel

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot requested review from andrewballantyne and jeff-phillips-18 March 27, 2020 21:55

openshift-ci-robot added component/core Related to console core functionality component/dev-console Related to dev-console component/shared Related to console-shared labels Mar 27, 2020

andrewballantyne suggested changes Mar 28, 2020

View reviewed changes

openshift-ci-robot assigned andrewballantyne Mar 28, 2020

sahil143 force-pushed the multi-stream-c branch 5 times, most recently from d40e2c0 to eb981b4 Compare March 30, 2020 20:43

andrewballantyne suggested changes Mar 30, 2020

View reviewed changes

andrewballantyne reviewed Mar 30, 2020

View reviewed changes

frontend/packages/console-shared/src/index.ts Outdated Show resolved Hide resolved

sahil143 force-pushed the multi-stream-c branch 2 times, most recently from 8e623fb to 5164ad5 Compare March 31, 2020 09:00

andrewballantyne suggested changes Mar 31, 2020

View reviewed changes

openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 31, 2020

sahil143 force-pushed the multi-stream-c branch from 5164ad5 to 5c731ca Compare March 31, 2020 21:33

andrewballantyne suggested changes Mar 31, 2020

View reviewed changes

sahil143 force-pushed the multi-stream-c branch 2 times, most recently from 50fe5c4 to 5ee3d02 Compare April 1, 2020 16:42

andrewballantyne suggested changes Apr 1, 2020

View reviewed changes

frontend/packages/console-shared/src/hooks/scroll.ts Outdated Show resolved Hide resolved

frontend/packages/console-shared/src/hooks/scroll.ts Outdated Show resolved Hide resolved

frontend/packages/dev-console/src/components/pipelineruns/logs/Logs.tsx Show resolved Hide resolved

sahil143 force-pushed the multi-stream-c branch from 5ee3d02 to b85ef2e Compare April 1, 2020 18:36

andrewballantyne approved these changes Apr 1, 2020

View reviewed changes

frontend/packages/console-shared/src/hooks/scroll.ts Outdated Show resolved Hide resolved

frontend/packages/dev-console/src/components/pipelineruns/logs/Logs.tsx Outdated Show resolved Hide resolved

andrewballantyne suggested changes Apr 1, 2020

View reviewed changes

sahil143 force-pushed the multi-stream-c branch from b85ef2e to 1cc013e Compare April 1, 2020 20:08

andrewballantyne approved these changes Apr 1, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 1, 2020

sahil143 force-pushed the multi-stream-c branch 4 times, most recently from 45744f7 to 60c86b6 Compare April 9, 2020 18:36

andrewballantyne suggested changes Apr 9, 2020

View reviewed changes

sahil143 force-pushed the multi-stream-c branch from 60c86b6 to 64da782 Compare April 9, 2020 18:55

andrewballantyne reviewed Apr 9, 2020

View reviewed changes

sahil143 force-pushed the multi-stream-c branch from 64da782 to 7099a0d Compare April 9, 2020 19:16

sahil143 force-pushed the multi-stream-c branch from 7099a0d to 0b33414 Compare April 9, 2020 19:37

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 10, 2020

sahil143 force-pushed the multi-stream-c branch 2 times, most recently from c1a1011 to eb7fcdd Compare April 12, 2020 20:01

sahil143 force-pushed the multi-stream-c branch from eb7fcdd to ac81f13 Compare April 12, 2020 20:09

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 12, 2020

andrewballantyne approved these changes Apr 13, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 14, 2020

openshift-merge-robot merged commit ded0639 into openshift:master Apr 14, 2020

spadgett added this to the v4.5 milestone Apr 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi-Stream log component #4845

Multi-Stream log component #4845

sahil143 commented Mar 27, 2020 •

edited

andrewballantyne left a comment

andrewballantyne Mar 30, 2020

andrewballantyne Apr 1, 2020

andrewballantyne Mar 31, 2020

andrewballantyne Mar 31, 2020

andrewballantyne commented Mar 31, 2020

andrewballantyne left a comment

andrewballantyne left a comment •

edited

andrewballantyne left a comment

christianvogt commented Apr 8, 2020

sahil143 commented Apr 9, 2020

andrewballantyne Apr 9, 2020

sahil143 Apr 9, 2020

andrewballantyne Apr 9, 2020 •

edited

sahil143 Apr 9, 2020

andrewballantyne Apr 9, 2020

andrewballantyne Apr 9, 2020

andrewballantyne Apr 9, 2020

andrewballantyne Apr 9, 2020

andrewballantyne Apr 9, 2020

sahil143 Apr 9, 2020

andrewballantyne left a comment

andrewballantyne commented Apr 9, 2020

sahil143 commented Apr 9, 2020

openshift-cherrypick-robot commented Apr 9, 2020

andrewballantyne commented Apr 9, 2020

openshift-cherrypick-robot commented Apr 9, 2020

andrewballantyne left a comment

christianvogt commented Apr 14, 2020

openshift-ci-robot commented Apr 14, 2020

openshift-cherrypick-robot commented Apr 14, 2020

	return obj ? <MultiStreamLogs taskName={taskName} resource={ref.current} /> : null;
	return ref.current ? <MultiStreamLogs taskName={taskName} resource={ref.current} /> : null;

	<span className="odc-multi-stream-logs__taskName--loading">
	<span className="odc-multi-stream-logs__loadingIndicator">

Multi-Stream log component #4845

Multi-Stream log component #4845

Conversation

sahil143 commented Mar 27, 2020 • edited

andrewballantyne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewballantyne commented Mar 31, 2020

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne left a comment • edited

Choose a reason for hiding this comment

andrewballantyne left a comment

Choose a reason for hiding this comment

christianvogt commented Apr 8, 2020

sahil143 commented Apr 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewballantyne Apr 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne commented Apr 9, 2020

sahil143 commented Apr 9, 2020

openshift-cherrypick-robot commented Apr 9, 2020

andrewballantyne commented Apr 9, 2020

openshift-cherrypick-robot commented Apr 9, 2020

andrewballantyne left a comment

Choose a reason for hiding this comment

christianvogt commented Apr 14, 2020

openshift-ci-robot commented Apr 14, 2020

openshift-cherrypick-robot commented Apr 14, 2020

sahil143 commented Mar 27, 2020 •

edited

andrewballantyne left a comment •

edited

andrewballantyne Apr 9, 2020 •

edited