chore(slo): Introduce domain models for SLO #142154

kdelemme · 2022-09-28T19:45:52Z

📝 Summary

This is an attempt to introduce a domain models so most of the business logic can be contained within the domain models instead of being spread into different services.

Types are necessary, and runtime validation with io-ts is great at the boundaries (rest api), but it cannot validate that the whole object is valid from a business perspective. For example, enforcing that the target objective is a number between ]0;1].
Domain models ensure the business rules are valid, and provide behaviours that abstract the business logic away from the consumer, e.g. we can imagine a method slo.replaceIndicator(newIndicator) which would replace the indicator but also bump the internal revision.

The Indicator domain models are derived directly from the io-ts schema. It's not ideal since the io-ts schemas can have some optional fields (in order to make the REST api lighter) than the domain models should enforce with some sane default value while being constructed.
But creating a proper domain model was also introducing a lot of boilerplate that the io-ts schema abstracted, thus, I decided to skip this part for the moment.

Now, I don't want to over-engineer this, and I'd like your thoughts on this.
Is it overkill? Do you have a simpler approach in mind? Should we just have two io-ts schemas, one for the rest api, and one for the domain models, with some commons and some domain function to provide behaviours?

This is an attempt to introduce a proper domain models so most of the business logic can be contained within the SLO domain models instead of being spread into different services. There is some drawback with the need to type the stored SLO that I need to investigate further

kibana-ci · 2022-09-28T20:01:15Z

💔 Build Failed

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #6 / machine learning - data frame analytics classification creation bank marketing displays the analytics job in the map view
[job] [logs] FTR Configs #6 / machine learning - data frame analytics classification creation bank marketing displays the analytics job in the map view
[job] [logs] FTR Configs #6 / machine learning - data frame analytics classification creation bank marketing edits the analytics job and displays it correctly in the job list
[job] [logs] FTR Configs #6 / machine learning - data frame analytics classification creation bank marketing edits the analytics job and displays it correctly in the job list
[job] [logs] FTR Configs #6 / machine learning - data frame analytics classification creation bank marketing navigates through the wizard and sets all needed fields
[job] [logs] FTR Configs #6 / machine learning - data frame analytics classification creation bank marketing navigates through the wizard and sets all needed fields
[job] [logs] FTR Configs #6 / machine learning - data frame analytics classification creation bank marketing runs the analytics job and displays it correctly in the job list
[job] [logs] FTR Configs #6 / machine learning - data frame analytics classification creation bank marketing runs the analytics job and displays it correctly in the job list
[job] [logs] FTR Configs #6 / machine learning - data frame analytics jobs cloning supported by UI form classification job supported by the form navigates through the wizard, checks and sets all needed fields
[job] [logs] FTR Configs #6 / machine learning - data frame analytics jobs cloning supported by UI form classification job supported by the form runs the clone analytics job and displays it correctly in the job list
[job] [logs] FTR Configs #6 / machine learning - data frame analytics outlier detection creation iowa house prices displays the analytics job in the map view
[job] [logs] FTR Configs #6 / machine learning - data frame analytics outlier detection creation iowa house prices edits the analytics job and displays it correctly in the job list
[job] [logs] FTR Configs #6 / machine learning - data frame analytics outlier detection creation iowa house prices edits the analytics job and displays it correctly in the job list
[job] [logs] FTR Configs #6 / machine learning - data frame analytics outlier detection creation iowa house prices navigates through the wizard and sets all needed fields
[job] [logs] FTR Configs #6 / machine learning - data frame analytics outlier detection creation iowa house prices runs the analytics job and displays it correctly in the job list
[job] [logs] FTR Configs #6 / machine learning - data frame analytics regression creation electrical grid stability displays the analytics job in the map view
[job] [logs] FTR Configs #6 / machine learning - data frame analytics regression creation electrical grid stability displays the analytics job in the map view
[job] [logs] FTR Configs #6 / machine learning - data frame analytics regression creation electrical grid stability edits the analytics job and displays it correctly in the job list
[job] [logs] FTR Configs #6 / machine learning - data frame analytics regression creation electrical grid stability edits the analytics job and displays it correctly in the job list
[job] [logs] FTR Configs #6 / machine learning - data frame analytics regression creation electrical grid stability navigates through the wizard and sets all needed fields
[job] [logs] FTR Configs #6 / machine learning - data frame analytics regression creation electrical grid stability navigates through the wizard and sets all needed fields
[job] [logs] FTR Configs #6 / machine learning - data frame analytics regression creation electrical grid stability runs the analytics job and displays it correctly in the job list
[job] [logs] FTR Configs #6 / machine learning - data frame analytics regression creation electrical grid stability runs the analytics job and displays it correctly in the job list

Metrics [docs]

Unknown metric groups

ESLint disabled in files

id	before	after	diff
`apm`	14	13	-1
`observability`	8	7	-1
total			-2

ESLint disabled line counts

id	before	after	diff
`apm`	81	79	-2
`observability`	45	44	-1
`securitySolution`	412	408	-4
`synthetics`	60	54	-6
`ux`	10	9	-1
total			-14

Total ESLint disabled count

id	before	after	diff
`apm`	95	92	-3
`observability`	53	51	-2
`securitySolution`	485	481	-4
`synthetics`	66	60	-6
`ux`	13	12	-1
total			-16

History

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

kdelemme · 2022-09-28T20:28:00Z

...plugins/observability/server/services/slo/transform_generators/apm_transaction_error_rate.ts

    return getSLOTransformId(slo.id);
  }

-  private buildSource(slo: APMTransactionErrorRateSLO) {
+  private buildSource(slo: SLO, indicator: APMTransactionErrorRateIndicator) {


💬 I have to provide the slo and the indicator separately otherwise the slo.indicator type cannot be inferred correctly. Except if I safeguard the slo.indicator.type in every private functions as done in the main function: https://github.com/elastic/kibana/pull/142154/files#diff-21eb1f91184393609c9483ec0f5c84d8cd0ee5ffb934065f338539bb79e2bad8R27.

kdelemme · 2022-09-28T20:29:35Z

x-pack/plugins/observability/server/types/rest_specs/slo.ts


 const createSLOParamsSchema = t.type({
-  body: commonSLOSchema,
+  body: t.type({


💬 rest_specs now contains only the API schema as well as the application service params (input) and response (output).

kdelemme · 2022-09-28T20:32:58Z

x-pack/plugins/observability/server/domain/models/slo.ts

+    if (objective.target > 1 || objective.target <= 0) {
+      throw new InvalidParameter('Invalid objective target');
+    }


💬 Business logic is enforced at creation, thus impossible state becomes impossible. Ideally, each value object should enforce their business rules, but in this case, Objective is a simple type derived from the io-ts schema.

kdelemme · 2022-09-28T20:33:29Z

x-pack/plugins/observability/server/services/slo/delete_slo.test.ts

@@ -29,7 +29,7 @@ describe('DeleteSLO', () => {

  describe('happy path', () => {
    it('removes the transform, the roll up data and the SLO from the repository', async () => {
-      const slo = createSLO(createAPMTransactionErrorRateIndicator());
+      const slo = createSLO();


💬 The createSLO fixture function uses an APMTransactionErrorRateIndicator by default.

kdelemme · 2022-09-29T13:02:41Z

Closing for the timebeing, found a more elegant solution

kdelemme added release_note:skip Skip the PR/issue when compiling release notes Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" v8.6.0 labels Sep 28, 2022

kdelemme force-pushed the chore/slo-domain-models branch from c044ebb to 71c1de1 Compare September 28, 2022 19:48

kdelemme commented Sep 28, 2022

View reviewed changes

kdelemme requested a review from a team September 28, 2022 20:45

kdelemme closed this Sep 29, 2022

kdelemme deleted the chore/slo-domain-models branch September 14, 2023 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(slo): Introduce domain models for SLO #142154

chore(slo): Introduce domain models for SLO #142154

kdelemme commented Sep 28, 2022 •

edited

Loading

kibana-ci commented Sep 28, 2022 •

edited

Loading

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

kdelemme Sep 28, 2022

kdelemme Sep 28, 2022

kdelemme Sep 28, 2022

kdelemme Sep 28, 2022

kdelemme commented Sep 29, 2022

chore(slo): Introduce domain models for SLO #142154

chore(slo): Introduce domain models for SLO #142154

Conversation

kdelemme commented Sep 28, 2022 • edited Loading

📝 Summary

kibana-ci commented Sep 28, 2022 • edited Loading

💔 Build Failed

Failed CI Steps

Test Failures

Metrics [docs]

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

kdelemme Sep 28, 2022

Choose a reason for hiding this comment

kdelemme Sep 28, 2022

Choose a reason for hiding this comment

kdelemme Sep 28, 2022

Choose a reason for hiding this comment

kdelemme Sep 28, 2022

Choose a reason for hiding this comment

kdelemme commented Sep 29, 2022

kdelemme commented Sep 28, 2022 •

edited

Loading

kibana-ci commented Sep 28, 2022 •

edited

Loading