Skip to content

[APM] POC: type-check aggregations based on runtime types#43080

Closed
dgieselaar wants to merge 1 commit intoelastic:masterfrom
dgieselaar:runtime-aggregation-types
Closed

[APM] POC: type-check aggregations based on runtime types#43080
dgieselaar wants to merge 1 commit intoelastic:masterfrom
dgieselaar:runtime-aggregation-types

Conversation

@dgieselaar
Copy link
Copy Markdown
Contributor

@dgieselaar dgieselaar commented Aug 11, 2019

We're currently exploring across observability how io-ts could help us. Infra + Logs added io-ts for their routes as well as for decoding Elasticsearch responses. io-ts is capable of generating both runtime checks and types from the same source. @sqren pointed out that this is a safer way of getting type safety as the runtime checks mean that our assumptions about Elasticsearch responses (which could be wrong) will be verified. However, leaving it up to the consumer to explicitly decode (and thus type) responses can (will) be cumbersome.

This is a quick demo of how we could have runtime checks + inferred type safety for ES queries from the same source of truth. It adds io-ts runtime types for both input and output of aggregations, and uses the TS types generated to check the ES query input and infer the type for its response.

Some thoughts:

  • it was fairly easy to update consumer code after I fixed the TS type. the metrics aggregations were kind of difficult, but that's more because of the way the code is structured.
  • I'm not sure whether runtime checks in prod on the entire ES response is a good idea. we might very well have a lot of situations where our io-ts types are incorrect, but our code can still successfully run, perhaps because we don't care about the part where our io-ts type is incorrect. One benefit of the explicit typing that infra+logs use is that the broadness of the check is less of a concern.
  • It would be interesting to see how things would look with explicit io-ts types to be able to compare both approaches properly. Any volunteers? 👻
  • the new APMSearchResponse type supports a io-ts type for hits as well, but I'm stubbing that right now because we'd otherwise have to create io-ts for all our documents.

(I didn't actually implement the runtime type checks for input & output, but I imagine the types are the hard part, and the runtime code should be fairly easy to implement (knocks on wood)).

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@dgieselaar
Copy link
Copy Markdown
Contributor Author

Closing in favor of #48394. IMHO, TypeScript is more expressive, and while io-ts is great, mirroring the TypeScript types that we have in io-ts codecs would be a significant effort.

@dgieselaar dgieselaar closed this Oct 16, 2019
@dgieselaar dgieselaar deleted the runtime-aggregation-types branch October 16, 2019 12:09
@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants