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

[discuss] Usage Collection: Should we use config-schema instead of our schema implementation? #85810

Closed
afharo opened this issue Dec 14, 2020 · 5 comments
Labels
Feature:Telemetry Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@afharo
Copy link
Member

afharo commented Dec 14, 2020

At the moment we are using our own implementation of the schema when declaring Usage Collectors. It helps us to define the structure of the data that we'll send to the Remote Telemetry Service in a very declarative way. We've also added some TS and tools that validate that the output types of the collector and the schema match to some degree.

Now that we are considering running actual validations (at least during CI) of the final payload based on that schema, would it be a good idea to use config-schema for that instead?

Pros:

  • Validation tools out of the box
  • Type-safe
  • Devs already know them because it's the preferred validation tool for HTTP API validation

Cons:

  • Breaking change
  • Level of definition (no distinction between number/long/short/float, ...
  • No straight JSON definition to share with the Remote Telemetry Service for contract-signing.

I'm creating this issue to track a discussion about it. A "no" is a perfectly valid answer to close this issue.

Related to #83704

@afharo afharo added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Telemetry labels Dec 14, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@pgayvallet
Copy link
Contributor

No straight JSON definition to share with the Remote Telemetry Service for contract-signing.

this seems to be the most impactful cons. Also config-schema supports complex / composite types that would (probably) not be convertible to simple types

schema.oneOf([schema.string(), schema.arrayOf(schema.object({...}))])

So I would say a more restrictive schema definition probably make sense here.

@Bamieh
Copy link
Member

Bamieh commented Jan 4, 2021

Thanks for opening an issue for this!

It does sound compelling but I am afraid this would restrict "growing" the schema api separately as needed. For example we might want to add a description field to the schema object in the future to get usage documentation out of the box.

Level of definition (no distinction between number/long/short/float, ...

I find this con the main reason to prevent us from switching. We do need this level of percision to understand the fields more and provide accurate es mappping. I dont see a replacement to having that if we switch.

No straight JSON definition to share with the Remote Telemetry Service for contract-signing.

I agree with @pgayvallet on this one. We do have a "translation" layer in the telemetry tool to convert the schema to the target json. However we'd have to keep some sort of feature parity with config-schema which means more maintenance work to keep things working.

@afharo
Copy link
Member Author

afharo commented Jan 7, 2021

Thank you for your comments! I totally agree with evolving our current schema is a better approach to migrating to config-schema. I just felt like this decision was never documented and wanted an issue where we could state the reasons for using our own implementation over anything else.

I'll leave this open for more comments until I start implementing #83704 (where I'll need to build a validation logic based on the schema).

@afharo
Copy link
Member Author

afharo commented Feb 9, 2021

Closing this issue as #90273 already added the logic to convert the Usage Collection's schema into config-schema to run validations.

@afharo afharo closed this as completed Feb 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Telemetry Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

4 participants