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][DQD][Tech Debt] Colocate components #190978

Merged
merged 13 commits into from
Aug 26, 2024

Conversation

kapral18
Copy link
Contributor

@kapral18 kapral18 commented Aug 21, 2024

addresses #190964

Second in the series of PRs to address general DQD tech debt. Currently this PR build on the work of #190970.

So this PR restructures components to colocate component throughout the hierarchy.

So instead of:

root/
  component1/
  childOfComponent1/
  grandChildOfComponent1/
  component2/
  use_hook_used_for_1/
  use_hook_used_for_2/

we use:

root/
  component1/
    hooks/
      use_hook_used_for_1/
    childOfComponent1/
      grandChildOfComponent1/
  component2/
    hooks/
      use_hook_used_for_2/

PROs of such scaffold:

  • complete and clear hierarchical visibility into component structure of the entire DQD codebase
  • ability to easily introduce and integrate a new change and calculate its impact on the tree of components
  • ability to easily remove colocated functionality without having to scout through the convoluted DQD code
  • clear understanding of where shared code should live as opposed to know when its shoved into top level by default with other non shared code
  • since nesting too deep has an import name readability tax it forces us to think about not splitting our components into too many small parts but rather keep it balanced, as opposed to now where flat structure incentivizes free and cheap fragmentation as seen with component like .

CONS:

  • import names have too many ../../../../../../../../../. It is fixable by ts paths/webpack aliases, but on the other hand especially if there are many of those it's an indication of potential architectural smell, that needs to be addressed (which is a PRO).

Imho, overall visibility trumps any cons and facilitates greater ease of adding new and changing existing functionality with more confidence.

Before

image

After

image

@kapral18 kapral18 self-assigned this Aug 21, 2024
@kapral18 kapral18 requested a review from a team as a code owner August 21, 2024 12:27
@kapral18 kapral18 added technical debt Improvement of the software architecture and operational architecture release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team Team:Threat Hunting:Explore v8.16.0 labels Aug 21, 2024
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@kapral18 kapral18 marked this pull request as draft August 21, 2024 12:36
@kapral18 kapral18 changed the title [Security Solution][DQD][Tech Debt] Restructure [Security Solution][DQD][Tech Debt] Colocate components Aug 21, 2024
@kapral18 kapral18 force-pushed the improve/DQD/190964-tech-debt-cleanup-2 branch from c67198f to 8a2e192 Compare August 21, 2024 20:41
@kapral18 kapral18 marked this pull request as ready for review August 22, 2024 18:24
@kapral18 kapral18 force-pushed the improve/DQD/190964-tech-debt-cleanup-2 branch from a1da4c8 to f4dd942 Compare August 22, 2024 18:26
@kapral18 kapral18 force-pushed the improve/DQD/190964-tech-debt-cleanup-2 branch from f4dd942 to 7aba7ce Compare August 23, 2024 23:20
addresses elastic#190964

First in the series of PRs to address general DQD tech debt

This one flattens and makes structure a little more logical

- rename top level data_quality/ folder to data_quality_panel/ in line
  with the actual component name in index.tsx
- remove redundant nested data_quality_panel/ folder
- Moved CheckStatus component under SummaryAction component
- Moved ErrorsViewer and ErrorsPopover components under `summary_actions/check_status`.
- remove body component and incorporate it into parent
- refactor data_quality_panel tests to incorporate the aforementioned
  change
- move IlmPhaseEmptyPrompt component
- move ErrorEmptyPrompt component
- move LoadingEmptyPrompt component
- move IndexResultBadge component
@kapral18 kapral18 force-pushed the improve/DQD/190964-tech-debt-cleanup-2 branch from 7aba7ce to 07fcb76 Compare August 26, 2024 10:41
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #69 / discover/context_awareness extension getDefaultAppState data view mode should merge and dedup configured default columns with default profile columns

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 5677 5676 -1

Async chunks

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

id before after diff
securitySolution 18.0MB 18.0MB +31.0B

History

  • 💔 Build #229606 failed 7aba7ce48b52aa545e8adc281b8dba0a8e615234
  • 💚 Build #229261 succeeded f4dd94280f4f879465e131db86d12da104d5c426
  • 💛 Build #228830 was flaky dd556494ffc9ce29ee43ac3ce213e45b14be061d

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

cc @kapral18

@kapral18 kapral18 merged commit dd18bc7 into elastic:main Aug 26, 2024
42 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Aug 26, 2024
@kapral18 kapral18 deleted the improve/DQD/190964-tech-debt-cleanup-2 branch August 26, 2024 12:29
kapral18 added a commit that referenced this pull request Aug 26, 2024
addresses #190964

Third in the series of PRs to address general DQD tech debt

This one builds on previous 2 PRs 

#190970
#190978 

Gist of changes:

- split top level helpers into series of utils/* files
- each utils/ file is named after common behavior it export or works
with.
- cleanup dead code
kapral18 added a commit that referenced this pull request Aug 28, 2024
…1245)

addresses #190964

Fourth in the series of PRs to address general DQD tech debt

This one builds on previous 3 PRs

#190970
#190978
#191233

Gist of changes:

split lower level helpers into series of utils/* files
each utils/ file is named after common behavior it export or works with.
cleanup dead code
kapral18 added a commit that referenced this pull request Aug 28, 2024
#191264)

addresses #190964

Fifth in the series of PRs to address general DQD tech debt

This one builds on previous 4 PRs

#190970
#190978
#191233
#191245

Gist of changes:

- split gigantic markdown helper file and colocate the parts where they
belong
- dedupe translations
- cleanup dead code
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:Threat Hunting:Explore Team:Threat Hunting Security Solution Threat Hunting Team technical debt Improvement of the software architecture and operational architecture v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants