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

[RAC][Rule Registry] Improve RuleDataService API and index bootstrapping implementation #108115

Merged

Conversation

banderror
Copy link
Contributor

@banderror banderror commented Aug 10, 2021

Addresses: #106421, #106428, #102089, #106433

Summary

This PR focuses on consolidation of indexing implementations in rule_registry (#101016). It addresses some of the sub-tasks of the parent ticket.

This will be addressed in follow-up PRs:

  • Enhance index bootstrapping: implement suggestions for backwards compatibility (naming scheme for alias and backing indices; versioning).
  • Enhance index bootstrapping: implement upgrades of existing index templates.
  • Make index bootstrapping logic more robust. This is partially addressed in this PR, but more improvements are needed.
  • Change the way index prefix works. UPD: [RAC] Disable RAC multi-tenancy #108506
  • Add support for optional TS schema (static typing).
  • Update README in rule_registry.

Checklist

@banderror banderror added Team:Detections and Resp Security Detection Response Team Theme: rac label obsolete labels Aug 10, 2021
@banderror banderror self-assigned this Aug 10, 2021
@banderror banderror force-pushed the improve-rule-data-service-and-client branch from a48f27d to a6fbb74 Compare August 10, 2021 21:29
@banderror banderror changed the title [RAC] Improve RuleDataService API and index bootstrapping implementation [RAC][Rule Registry] Improve RuleDataService API and index bootstrapping implementation Aug 10, 2021
@banderror banderror force-pushed the improve-rule-data-service-and-client branch from d799fa8 to 8d75711 Compare August 11, 2021 01:00
@banderror banderror marked this pull request as ready for review August 11, 2021 01:45
@banderror banderror requested review from a team as code owners August 11, 2021 01:45
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@banderror banderror added release_note:skip Skip the PR/issue when compiling release notes v7.15.0 v8.0.0 Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels Aug 11, 2021
@elasticmachine
Copy link
Contributor

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

@botelastic botelastic bot added Team:APM All issues that need APM UI Team support Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Aug 11, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

componentTemplates: [
{
name: 'mappings',
version: 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Proper versioning according to the recent discussions will be implemented in a follow-up PR.

Comment on lines +103 to +113
const { ruleDataService } = plugins.ruleRegistry;
const ruleDataClient = ruleDataService.initializeIndex({
feature: 'observability',
registrationContext: 'observability',
dataset: Dataset.alerts,
componentTemplateRefs: [],
componentTemplates: [],
indexTemplate: {
version: 0,
},
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This replaces the previous implementation:

    const ruleDataClient = plugins.ruleRegistry.ruleDataService.getRuleDataClient(
      'observability',
      plugins.ruleRegistry.ruleDataService.getFullAssetName(),
      () => Promise.resolve()
    );

Seems like it's a stub. Should I keep it? Why it's needed?

Copy link
Member

Choose a reason for hiding this comment

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

The observability plugin apparently needs a ruleDataClient for use in some route handlers without intending to write any alerts itself. Does this initializeIndex create any assets which the previous implementation didn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this initializeIndex create any assets which the previous implementation didn't?

It will see if it should bootstrap any resources for .alerts-observability. Specifically:

  • ILM policy if it's provided. In this case, it's not.
  • Component templates if they are provided. In this case, they're not.

And then it will update mappings of all concrete indices matching .alerts-observability-*. This pattern shouldn't match any indices, I guess, because you're saying this ruleDataClient is not used for writing alerts? Other indices containing alerts are named like .alerts-observability.apm-* etc.

Is this ruleDataClient used for reading all the observability alerts - from .alerts-observability.apm-*, .alerts-observability.logs-* and other indices?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems for this use case it would be useful to expose some kind of getRuleDataReader API in RuleDataPluginService that skips all the index management steps and just returns an IRuleDataReader. If I understand correctly the registrationContext would be observability.*.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also thinking about that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a ticket for that: #111173

@banderror banderror added the auto-backport Deprecated - use backport:version if exact versions are needed label Aug 11, 2021
Copy link
Contributor

@marshallmain marshallmain left a comment

Choose a reason for hiding this comment

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

I went through and did a first pass of the changes here. I found a few ways that I think we can simplify the implementation further, especially regarding signalling when asynchronous template installation functions are complete and the next operations can proceed.

But, very exciting to see the new usage of ruleDataService.initializeIndex! It's a huge improvement to move the initialization and signalling logic into a single place rather than having each plugin implement it separately.

@banderror banderror force-pushed the improve-rule-data-service-and-client branch from 6895ff9 to f625838 Compare August 11, 2021 16:29
@banderror banderror force-pushed the improve-rule-data-service-and-client branch from adfe310 to 077ca24 Compare August 15, 2021 10:13
@banderror banderror merged commit 2ee11db into elastic:master Aug 15, 2021
@banderror banderror deleted the improve-rule-data-service-and-client branch August 15, 2021 12:53
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 15, 2021
…ing implementation (elastic#108115)

**Addresses:** elastic#106421, elastic#106428, elastic#102089, elastic#106433

## Summary

This PR focuses on consolidation of indexing implementations in `rule_registry` (elastic#101016). It addresses some of the sub-tasks of the parent ticket.

- [x] Encapsulate index bootstrapping logic in a new improved API exposed by `RuleDataService`.
- [x] Enforce allowed values for the `datasetSuffix` on the API level.
- [x] Migrate plugins using the existing `RuleDataService` API to the improved one.
- [x] Make sure index names comply with design architecture.
    - elastic#102089
- [x] Improve the API of `RuleDataClient`.
- [x] Enhance index bootstrapping: support custom ILM policy per index (`{registrationContext}.{datasetSuffix}`).
- [x] Enhance index bootstrapping: create index template per namespace and support rollovers properly
    - based on elastic#107700
- [x] Enhance index bootstrapping: support secondary aliases
    - based on elastic#107700
- [x] Remove `EventLogService` implementation
    - elastic#106433

This will be addressed in follow-up PRs:

- [ ] Enhance index bootstrapping: implement suggestions for backwards compatibility (naming scheme for alias and backing indices; versioning).
- [ ] Enhance index bootstrapping: implement upgrades of existing index templates.
- [ ] Make index bootstrapping logic more robust. This _is partially addressed_ in this PR, but more improvements are needed.
- [ ] Change the way index prefix works.
- [ ] Add support for optional TS schema (static typing).
- [ ] Update `README` in `rule_registry`.

### Checklist

- [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials
- [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
@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 Aug 15, 2021
…ing implementation (#108115) (#108638)

**Addresses:** #106421, #106428, #102089, #106433

## Summary

This PR focuses on consolidation of indexing implementations in `rule_registry` (#101016). It addresses some of the sub-tasks of the parent ticket.

- [x] Encapsulate index bootstrapping logic in a new improved API exposed by `RuleDataService`.
- [x] Enforce allowed values for the `datasetSuffix` on the API level.
- [x] Migrate plugins using the existing `RuleDataService` API to the improved one.
- [x] Make sure index names comply with design architecture.
    - #102089
- [x] Improve the API of `RuleDataClient`.
- [x] Enhance index bootstrapping: support custom ILM policy per index (`{registrationContext}.{datasetSuffix}`).
- [x] Enhance index bootstrapping: create index template per namespace and support rollovers properly
    - based on #107700
- [x] Enhance index bootstrapping: support secondary aliases
    - based on #107700
- [x] Remove `EventLogService` implementation
    - #106433

This will be addressed in follow-up PRs:

- [ ] Enhance index bootstrapping: implement suggestions for backwards compatibility (naming scheme for alias and backing indices; versioning).
- [ ] Enhance index bootstrapping: implement upgrades of existing index templates.
- [ ] Make index bootstrapping logic more robust. This _is partially addressed_ in this PR, but more improvements are needed.
- [ ] Change the way index prefix works.
- [ ] Add support for optional TS schema (static typing).
- [ ] Update `README` in `rule_registry`.

### Checklist

- [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials
- [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

Co-authored-by: Georgii Gorbachev <georgii.gorbachev@elastic.co>
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

API count

id before after diff
ruleRegistry 95 114 +19

API count missing comments

id before after diff
ruleRegistry 95 94 -1

Non-exported public API item count

id before after diff
ruleRegistry 11 4 -7

History

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

cc @banderror

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:APM All issues that need APM UI Team support Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability Theme: rac label obsolete v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants