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

[Security Solution][Endpoint] Add an additional hint message for Console commands pending more than 15s #135500

Conversation

paul-tavares
Copy link
Contributor

Summary

  • When a command is pending for more than 15s, show an additional hint message under the pending message
  • New component for displaying console output text
  • Refactored CommandExecutionResult to use new text component

olm-4272-console-long-running-message

Checklist

@paul-tavares paul-tavares added release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.4.0 labels Jun 29, 2022
@paul-tavares paul-tavares requested a review from a team as a code owner June 29, 2022 23:14
@paul-tavares paul-tavares self-assigned this Jun 29, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt)

@paul-tavares
Copy link
Contributor Author

Will work on tests tomorrow

Copy link
Contributor

@dasansol92 dasansol92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚢

Copy link
Member

@ashokaditya ashokaditya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works as expected. I just have a few comments.

I noticed this odd glitch that is probably unrelated to this PR. The console flickers whenever there is status output on the console. Here's screen clip in slow-mo. Probably cause it's calling and waiting on the status API to return.

status-flicker

Comment on lines +73 to +83
const elapsedSeconds = moment().diff(moment(enteredAt), 'seconds');

if (elapsedSeconds >= 15) {
setIsLongRunningCommand(true);
return;
}

timeoutId = setTimeout(() => {
setIsLongRunningCommand(true);
}, (15 - elapsedSeconds) * 1000);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here the elapsedSeconds never increments or is always 0. Basically, the timeout is set to execute after 15 seconds and sets the isLongRunningCommand to true. We're not actually using the time difference here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you mean.

Let me explain this logic around elapsedSeconds. You can enter a command and then close the Responder window... When we re-open that same responder window, we need to determine how much time has elapsed since the command was entered so that we can correctly calculate the setTimeout() ms.

Example:

  • I enter a command at :00 s and immediately close the console
  • I reopen the console just 5s later.
  • the setTimeout() should now only wait 10s since 5s has already elapsed... so the (15 - elapsedSeconds) does that.
  • This logic here will only be triggered if 15s has not yet elapsed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see. When the command is executed and the console stays open beyond 15 seconds the elapsedSeconds is always 0 and thus the elapsedSeconds >= 15 block does not execute is what I meant. I didn't test it by closing and then re-opening the console, in which case the block does execute and it works as expected.

paul-tavares and others added 3 commits June 30, 2022 09:49
From Ash

Co-authored-by: Ashokaditya <1849116+ashokaditya@users.noreply.github.com>
@paul-tavares paul-tavares requested a review from ashokaditya June 30, 2022 15:24
@paul-tavares
Copy link
Contributor Author

@ashokaditya thanks for pointing out that "flicker" issue. I just looked at the status command and it has not been updated to use the "loading" message. I will create an issue for tracking and will make the quick change

Copy link
Member

@ashokaditya ashokaditya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢 it!

@kevinlog kevinlog enabled auto-merge (squash) July 6, 2022 15:31
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 3108 3110 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 5.2MB 5.2MB +703.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @paul-tavares

@kevinlog kevinlog merged commit 092c8e1 into elastic:main Jul 6, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 6, 2022
@paul-tavares paul-tavares deleted the task/olm-4272-show-hint-on-long-running-actions branch July 11, 2022 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants