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

[Infra] Replace MetricsUIAggregation in favor of estypes.AggregationsAggregate #190311

Closed
crespocarlos opened this issue Aug 12, 2024 · 1 comment · Fixed by #190495
Closed

[Infra] Replace MetricsUIAggregation in favor of estypes.AggregationsAggregate #190311

crespocarlos opened this issue Aug 12, 2024 · 1 comment · Fixed by #190495
Assignees
Labels
Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team technical debt Improvement of the software architecture and operational architecture

Comments

@crespocarlos
Copy link
Contributor

crespocarlos commented Aug 12, 2024

Summary

In Infra, there is extensive code for typing Elasticsearch request parameters using io-ts. This code was created a few years ago (git history), before the Elasticsearch client implemented ts types.

While one of the benefits of using io-ts is runtime validation, in this case, the benefit doesn't outweigh the cost of maintaining this code. We might be better off relying on the types provided by the elasticsearch library.

AC

  • Replace MetricsUIAggregation in favor of estypes.AggregationsAggregate
  • MetricsUIAggregation related types definitions are removed
@crespocarlos crespocarlos added the technical debt Improvement of the software architecture and operational architecture label Aug 12, 2024
@botelastic botelastic bot added the needs-team Issues missing a team label label Aug 12, 2024
@crespocarlos crespocarlos added Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team and removed technical debt Improvement of the software architecture and operational architecture needs-team Issues missing a team label labels Aug 12, 2024
@elasticmachine
Copy link
Contributor

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

@crespocarlos crespocarlos added the technical debt Improvement of the software architecture and operational architecture label Aug 12, 2024
@crespocarlos crespocarlos changed the title [Infra] Deprepacte MetricsUIAggregation in favor of estypes.AggregationsAggregate [Infra] Replace MetricsUIAggregation in favor of estypes.AggregationsAggregate Aug 12, 2024
@jennypavlova jennypavlova self-assigned this Aug 12, 2024
stephmilovic pushed a commit to stephmilovic/kibana that referenced this issue Aug 21, 2024
…onsAggregate` (elastic#190495)

Closes [elastic#190311](elastic#190311)

## Summary

This PR replaces `MetricsUIAggregation` in favor of
`estypes.AggregationsAggregate`. Now the `MetricsUIAggregation` uses
estypes.AggregationsAggregate and the `MetricsUIAggregationRT `is
removed which also allows us to remove the `ESAggregationRT`. Instead of
maintaining the runtime types we now rely on the types provided by
elasticsearch.

A follow-up issue will be linked here to address the other aggregation
types related changes:
[elastic#190497](elastic#190497)

## Testing
- Check the types 
- Host page should load with no errors
- To test if the API works use the request provided in the
[x-pack/plugins/observability_solution/infra/server/routes/infra/README.md](https://github.com/elastic/kibana/compare/main...jennypavlova:kibana:190311-infra-replace-metricsuiaggregation-in-favor-of-estypesaggregationsaggregate?expand=1#diff-e853fdd3f4073eff8ff8a4df6a657f8cb5fefaa231be0116fe3692ae929f26a8)
- if the body is not valid 400 is returned

---------

Co-authored-by: kibanamachine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team technical debt Improvement of the software architecture and operational architecture
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants