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

[SecuritySolution] Replace KPIs on Host, Network, and Users with Lens Embeddables #138369

Merged
merged 176 commits into from
Nov 9, 2022

Conversation

angorayc
Copy link
Contributor

@angorayc angorayc commented Aug 9, 2022

Summary

This is part of #136409
In this PR we replace KPIs on Host, Network, and Users with Lens Embeddables.

Feature flag: chartEmbeddablesEnabled
Please add xpack.securitySolution.enableExperimental: [chartEmbeddablesEnabled] to kibana.dev.yml to verify

There shouldn't be much difference between Lens Embeddables and Elastic Charts. This migration is more like a tech debt as Lens Embeddables have implemented the functionality we need from fetching data to UI display, there's no need for us to implement / maintain the same functionality again on our side.
From the UI side, users might find that some bar charts and area charts (e.g.: source and dest. IP, authentication success and failed) now have two requests and responses in the inspector. That's expected, it's just because they are query separately in Lens Embeddables.

Eventually we'd like to replace all the charts on Security Solution with Lens Embeddable. However, this scolp is too big to ship in a single release and we would like to have more testing before we're confident enough, we put it behind the feature flag for now.

To test:

  1. Apply global queries and filters
  2. Apply indices from sourcerer
  3. Hitting global refreshing button
  4. Update time range and refetch by brushing on area charts
  5. Actions in ... button
Screen.Recording.2022-10-27.at.15.11.41.mov

Before

Screenshot 2022-08-11 at 13 06 47

Checklist

Delete any items that are not applicable to this PR.

@angorayc angorayc mentioned this pull request Aug 9, 2022
9 tasks
@angorayc angorayc marked this pull request as ready for review November 7, 2022 09:18
@angorayc angorayc requested review from a team as code owners November 7, 2022 09:18
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

deleteQuery,
inspect,
loading,
queryId: id,
refetch: isChartEmbeddablesEnabled ? refetchByRestartingSession : refetch ?? noop, // refetchByRestartingSession is for refetching Lens Embeddables
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to use this new refetchByRestartingSession function instead of the original refetch? Just to understand.
Also, manageQuery is used in many different places, some are not related to any lens visualizations, do we need to change the refetch to all those places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lens Embeddables do not have a function we can call to refetch data like most of our components, it refetches when receiving a new search session ID. So in x-pack/plugins/security_solution/public/common/components/page/use_refetch_by_session.tsx Line 28 , we assign it the initial session ID, it's consumed in x-pack/plugins/security_solution/public/common/components/visualization_actions/lens_embeddable.tsx. It gets new session ID every time when refetching: x-pack/plugins/security_solution/public/common/components/page/use_refetch_by_session.tsx Line37

Yeah, and I think I should not have changed manage_query.
I put it in x-pack/plugins/security_solution/public/common/components/page/manage_query.tsx was trying not to repeat the implementation, but I think it makes more sense to be in the same place where each component gets their refetch function. Take KPI hosts as an example, I'm going to move useRefetchByRestartingSession to x-pack/plugins/security_solution/public/hosts/components/kpi_hosts/hosts/index.tsx

const [loading, { refetch, id, inspect, ...data }] = useHostsKpiHosts({
    filterQuery,
    endDate: to,
    indexNames,
    startDate: from,
    skip: querySkip || isChartEmbeddablesEnabled,
  });

  const { searchSessionId, refetchByRestartingSession } = useRefetchByRestartingSession({
    inputId: InputsModelId.global,
    queryId: id,
  });

  return (
    <KpiBaseComponentManage
      data={data}
      id={id}
      inspect={inspect}
      loading={loading}
      fieldsMapping={fieldsMapping}
      from={from}
      to={to}
      updateDateRange={updateDateRange}
      refetch={isChartEmbeddablesEnabled ? refetchByRestartingSession : refetch}
      setQuery={setQuery}
      setQuerySkip={setQuerySkip}
      searchSessionId={isChartEmbeddablesEnabled ? searchSessionId : undefined}
    />
  );

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that looks better 👍 Thanks for the explanation!

@machadoum
Copy link
Member

@angorayc I noticed some weird-looking UI. Let me know if it is a problem with my environment.

When I open the host page I get an error and dark background behind the scroll bar and a black square in the header.
Screenshot 2022-11-07 at 17 29 14

When I open it on a smaller screen:
Loading state
Screenshot 2022-11-07 at 17 28 28
Empty state:
Screenshot 2022-11-07 at 17 28 47
When loaded:
Screenshot 2022-11-07 at 17 29 02

@angorayc
Copy link
Contributor Author

angorayc commented Nov 8, 2022

Hey @machadoum , thanks for catching this. I think that's because source.ip and destination.ip both have conflict in their field. Lens doesn't seem to allow aggregation on a non keyword type, but our current charts are retrieving data from Elasticsearch api, which doesn't show error in this case. Guessing that's why we have the inconsistency between current charts and Lens Embeddables. I'm asking Lens' opinion on how to fix this https://elastic.slack.com/archives/C013C7Z2X8W/p1667905889860969
Update: Lens team has this issue open: #143673

Screenshot 2022-11-08 at 10 45 50

Screenshot 2022-11-08 at 10 47 44

Screenshot 2022-11-08 at 10 48 32

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Security Solution Tests #1 / Cases Creates a new case with timeline and opens the timeline

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 3204 3214 +10

Async chunks

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

id before after diff
securitySolution 9.6MB 9.6MB +10.7KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
securitySolution 50.6KB 50.6KB +27.0B
Unknown metric groups

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 59 65 +6
osquery 108 113 +5
securitySolution 440 446 +6
total +19

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 67 73 +6
osquery 109 115 +6
securitySolution 517 523 +6
total +20

History

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

@angorayc angorayc added the backport:skip This commit does not require backporting label Nov 9, 2022
@angorayc angorayc merged commit 48bbfdc into elastic:main Nov 9, 2022
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: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore Team:Threat Hunting Security Solution Threat Hunting Team v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants