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

[SLO] Auto generating OpenAPI Spec (OAS) using @kbn/config-schema for a sample SLO API #184138

Closed
maryam-saeidi opened this issue May 23, 2024 · 8 comments
Assignees
Labels
Feature:OAS Work or issues related to Core-provided mechanisms for generating OAS Feature:SLO R&D Research and development ticket (not meant to produce code, but to make a decision) Team:obs-ux-management Observability Management User Experience Team

Comments

@maryam-saeidi
Copy link
Member

maryam-saeidi commented May 23, 2024

🍒 Summary

As mentioned in this email, platform team worked on introducing a way to automatically generate OpenAPI Spec (OAS) using @kbn/config-schema.

Here is their first iteration on the /api/status API. (PR)

In this ticket, we would like to use @kbn/config-schema for one of the SLO APIs and compare the result with the existing OpenAPI Spec to figure out if something is missing and, if not, how much effort is needed for this migration.

Here is the existing list of SLO OpenAPI Specifications. We need to decide which SLO API is a good candidate based on the complexity, to make sure our estimates are realistic.

✅ Acceptance criteria

  • Auto-generate Open API Spec for one of the complicated SLO APIs and compare it with existing specifications. (Write down any challenges or limitations in this process)
  • Provide an estimation of how much work it would be to migrate all SLO API specifications.
@maryam-saeidi maryam-saeidi added Feature:SLO Team:obs-ux-management Observability Management User Experience Team Feature:OAS Work or issues related to Core-provided mechanisms for generating OAS labels May 23, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-management-team (Team:obs-ux-management)

@maryam-saeidi maryam-saeidi added the R&D Research and development ticket (not meant to produce code, but to make a decision) label May 28, 2024
@maryam-saeidi maryam-saeidi self-assigned this May 28, 2024
@maryam-saeidi
Copy link
Member Author

maryam-saeidi commented Jun 4, 2024

In this comment, I will log the findings that I have related to changing a sample validation to using @kbn/config-schema

I've started with Delete /api/observability/slos/{id} first to see how the flow works and I will move to a more complicated API in the next step.

Config schema documentation

https://github.com/elastic/kibana/blob/main/packages/kbn-config-schema/README.md

Implementation

After changing registering a route with @kbn/config-schema validation, and enabling server.oas.enabled: true in kibana.dev.yaml (as mentioned in this PR), I had 2 options to see related OAS specifications:

  1. Using pluginId: http://localhost:5601/api/oas?pluginId=@kbn/slo-plugin
  2. Using pathStartsWith: http://localhost:5601/api/oas?pathStartsWith=/api/observability/slos

Next, I tried to recreate deleteSloOp as specified here.

image

Findings

  1. The pluginId seemed to have a bug, and we need to specify the package ID instead. (issue)
  2. The Description field was actually the summary field in OAS; here is the PR for fixing this mismatch.
  3. Example is not supported yet. (There is an item for this in this issue)
  4. Config-schema does not support partial for an object. It provides schema.maybe that can be used with every field separately.
    We don't need to separate partial vs mandatory fields similar to io-ts when we use config-schema. The only case that might be useful to have partial is when we already have a type but in some scenarios we would like to allow passing partial objects, like:
  5. Config-schema support for intersection is a bit verbose: (I've created an improvement ticket for intersection)
    const a = schema.object({ a: schema.string() });
    const bKeys = { b: schema.string() };
    const b = schema.object(bKeys);
    
    // C = A & B
    const c = a.extends(bKeys);
    

@simianhacker
Copy link
Member

simianhacker commented Jun 4, 2024

I see you're converting a schema that's pretty simple, Delete /api/observability/slos/{id}. I think where this becomes really tricky is that the other schemas re-use io-ts schema for most of the bits and pieces which are also used in our models, like the updateSLOParamsSchema. Does that mean we are going to have to convert everything to config-schema?

The issue I have with this effort is that there isn't feature parity yet and I'd hate have to maintain 2 sets of schemas. I love the promise of mostly getting the OpenAPI docs for free by using the code but I think we might need to push back until the replacement library is up to par with the io-ts features we use. The features that come to mind:

  • Runtime types that are reused to create Typescript types
  • Type guards using schemas throughout the code
  • Custom types like the Duration type
  • Documentation for io-ts with examples

@maryam-saeidi
Copy link
Member Author

maryam-saeidi commented Jun 5, 2024

@simianhacker Thanks for sharing the list of features that we need to evaluate. Indeed, the delete API was just a starting point for me to see how OAS works in action, but the main goal is to evaluate the gaps that we might have between config-schema and io-ts. I will use the list that you provided as a guide, thanks.

@jasonrhodes
Copy link
Member

What would be the best endpoint to choose as the next one, to highlight the gaps as starkly as possible? As Mary said, the goal here isn't to start a migration, but rather to help expose the gaps so we can provide feedback to the platform team re: config-schema limitations. The more we look at this, the less likely it seems we'd want to spend time on converting/refactoring lots of code just for auto-generated OpenAPI specs.

@kdelemme
Copy link
Contributor

kdelemme commented Jun 5, 2024

The update or find APIs would be very good candidate:

  • The update API because the indicator type is complex
  • The find API because the response is used everywhere in the app, and part of its schema is used for runtime validation, e.g. occurrencesBudgetingMethod.is(slo.budgetingMethod)

@maryam-saeidi
Copy link
Member Author

maryam-saeidi commented Jun 7, 2024

Based on the above discussions, here is a list of items that need to be verified and related investigation: (Related POC)

1. Runtime types that are reused to create Typescript types

It is possible to return type a schema using TypeOf<typeof rt>, For example:

type CreateSLOInputConfigSchema = TypeOf<typeof createSLOParamsConfigSchema.body>

The challenge comes from supporting type transformation, which will be discussed in the third section, the Duration type.

2.Type guards using schemas throughout the code

At the moment, in the cases that we use Type guard (like the example that Kevin shared, e.g. occurrencesBudgetingMethodSchema.is(slo.budgetingMethod)), we only use the validation part of the is functionality as far as I see. For those, we could use validate function, but if the type does not match, then it throws an error which is not the expected behaviour and we need to maybe have a wrapper around it if we don't want to throw an error.

🗒️ I've created a ticket for adding support for is type checking.

3.Custom types like the Duration type

For custom types, the validation can be done using config schema custom validation. The challenge is about converting string -> Duration and having proper input and output types as defined here.

🗒️ I've created a ticket to add transform functionality to the config-schema.

4. Documentation with examples

Here is schema-config documentation which has lots of good examples. Maybe the only challenge is how to make it easier for developers to find this documentation and ensure it is up to date.

🗒️ I've created a ticket to cover adding extends to the documentation which was missing.

Summary of related tickets

@maryam-saeidi
Copy link
Member Author

Closing this ticket as the investigation is completed, and we decided not to change SLO's API params validation and rely on manual OAS since the API does not change that often.

Also, we introduce Zod as an alternative to config-schema for automatically generating OAS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:OAS Work or issues related to Core-provided mechanisms for generating OAS Feature:SLO R&D Research and development ticket (not meant to produce code, but to make a decision) Team:obs-ux-management Observability Management User Experience Team
Projects
None yet
Development

No branches or pull requests

5 participants