Skip to content

[Security Solution][Telemetry] Refactoring security telemetry task code#114095

Merged
tsouza merged 27 commits intoelastic:masterfrom
tsouza:security-task-refactor
Oct 12, 2021
Merged

[Security Solution][Telemetry] Refactoring security telemetry task code#114095
tsouza merged 27 commits intoelastic:masterfrom
tsouza:security-task-refactor

Conversation

@tsouza
Copy link

@tsouza tsouza commented Oct 6, 2021

Summary

Over the past releases we've introduced a few Kibana task classes for supporting security solution telemetry. In its current versions there seems to be some code duplication. This PR is an effort for refactoring these classes and simplify things.

Change summary:

  1. Removed all duplicate/redundant code from the task classes defined in tasks and consolidated them in a single task class defined in task.ts. The code that remained within tasks is the logic that is particular to each telemetry task.
  2. Wrote a new test suite in task.test.ts for the new reusable task class defined in task.ts. The new test suite uses a dummy task definition and it tests basic capabilities such as being able to register a task and decide if a task should run or not depending if telemetry is opted in or out.
  3. Simplified the test for the task in tasks. The tests now focus on the specific logic for each class. For now, the tests only verifies if methods that fetches data is being properly called. This is because the telemetry data structure that the telemetry classes work with are fairly complex and it would be time consuming to write sample/fake versions of this data to properly test other method calls. We should revisit and improve tests in a future iteration.

To Do:

  • Improve task tests.
  • The TelemetryEventsSender class still spawns a rogue telemetry channel that runs entirely different from the other task. We should also try to bring this into Kibana task manager.

Checklist

Delete any items that are not applicable to this PR.

@tsouza tsouza added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. auto-backport Deprecated - use backport:version if exact versions are needed v7.16.0 v7.15.1 labels Oct 6, 2021
@tsouza tsouza requested a review from pjhampton October 8, 2021 16:45
@tsouza tsouza marked this pull request as ready for review October 8, 2021 16:45
@tsouza tsouza requested a review from a team as a code owner October 8, 2021 16:45
@elasticmachine
Copy link
Contributor

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

@tsouza tsouza changed the title [Security Solution][Telemetry] [WIP] Refactoring security telemetry task code [Security Solution][Telemetry] Refactoring security telemetry task code Oct 8, 2021
@tsouza
Copy link
Author

tsouza commented Oct 8, 2021

@elasticmachine merge upstream

@pjhampton
Copy link
Contributor

@elasticmachine merge upstream

@pjhampton pjhampton removed the v7.15.1 label Oct 11, 2021
Copy link
Contributor

@pjhampton pjhampton left a comment

Choose a reason for hiding this comment

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

I really like where this is going. Great job! 🚀

Can't really fault it. I pulled down your code and ran it against ES. Tasks still run as expected and opt-in/out functionality working as expected.

Please address my comment and incorporate #113239 when it is to be merged today! ✨

@tsouza
Copy link
Author

tsouza commented Oct 11, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

@tsouza tsouza requested a review from pjhampton October 12, 2021 11:21
Copy link
Contributor

@pjhampton pjhampton left a comment

Choose a reason for hiding this comment

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

🌔 🚀 ✨ LGTM ✨ 🚀 🌔

@tsouza
Copy link
Author

tsouza commented Oct 12, 2021

@elasticmachine merge upstream

@tsouza tsouza enabled auto-merge (squash) October 12, 2021 22:24
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@tsouza tsouza merged commit fb44147 into elastic:master Oct 12, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Oct 12, 2021
…de (elastic#114095)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Oct 13, 2021
…de (#114095) (#114738)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Thiago Souza <thiago@elastic.co>
@tsouza tsouza deleted the security-task-refactor branch October 13, 2021 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.16.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants