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 Solutions] Removes POC transforms #129673

Merged
merged 8 commits into from
Apr 11, 2022

Conversation

FrankHassanabad
Copy link
Contributor

@FrankHassanabad FrankHassanabad commented Apr 6, 2022

Summary

Removes the metrics_entities plugin and POC. As a different direction will be taken and people can look back at the git history for it as they see fit if they need to refer to it. Once it's re-added it it will be through an RFC process and re-discussed.

Earlier PR's which added the POC:

#96446
#104559

Checklist

@FrankHassanabad FrankHassanabad self-assigned this Apr 6, 2022
@FrankHassanabad FrankHassanabad changed the title Remove transform [Security Solutions] Removes POC transforms Apr 6, 2022
@FrankHassanabad FrankHassanabad added v8.3.0 backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes labels Apr 6, 2022
@FrankHassanabad FrankHassanabad marked this pull request as ready for review April 7, 2022 14:36
@FrankHassanabad FrankHassanabad requested review from a team as code owners April 7, 2022 14:36
@dhurley14
Copy link
Contributor

I probably have not been involved in some conversations but I'm wondering why not just amend the code as we flesh out features for this rather than pull it all out now? Is there detriment in keeping it in the code base for now? If there have been conversations around just re-building from the ground up and this is the approach I think that is fair but I was just wondering if there are any discussions I missed around pulling this out of the codebase now?

@FrankHassanabad
Copy link
Contributor Author

FrankHassanabad commented Apr 7, 2022

@dhurley14, It's been in the codebase for almost a year with no traction. Since I'm departing, I don't think I should leave behind code that would be difficult for another engineer to remove at this point vs. the other way which is an engineer can re-invent the code or copy the code back if need be.

This feature requires a RFC and probably a re-think and renewed ownership. The code can be found in the git history if needed. It would de-activate in several instances and is hard to explain to people how metrics and entities work generally. Looking at our roadmap this performance improvement honestly doesn't look like it is ever going to be a priority vs. other features? I'm just pointing out that maybe the business is ok with the performance of the application as it is today. If this is not going to be a priority for another year or ever and probably never worked on again why keep it in the codebase was my thinking.

I mean, at this point it's up to you, @yctercero, and @peluja1012 if you want to keep it in here or not.

@FrankHassanabad
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 2782 2765 -17

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
metricsEntities 4 - -4

Async chunks

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

id before after diff
securitySolution 4.8MB 4.8MB -8.2KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
metricsEntities 1 - -1

Page load bundle

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

id before after diff
securitySolution 251.8KB 250.3KB -1.4KB
Unknown metric groups

API count

id before after diff
metricsEntities 4 - -4

ESLint disabled line counts

id before after diff
metricsEntities 1 - -1
securitySolution 446 445 -1
total -2

Total ESLint disabled count

id before after diff
metricsEntities 1 - -1
securitySolution 515 514 -1
total -2

History

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

cc @FrankHassanabad

Copy link
Contributor

@dhurley14 dhurley14 left a comment

Choose a reason for hiding this comment

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

Discussed with everyone and agreed on general direction of relying on git history when the time comes to implementing metrics entities etc. LGTM.

@FrankHassanabad FrankHassanabad merged commit ce2f617 into elastic:main Apr 11, 2022
@FrankHassanabad FrankHassanabad deleted the remove-transform branch April 11, 2022 19:41
@LeeDr LeeDr added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Apr 25, 2022
@elasticmachine
Copy link
Contributor

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

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. v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants