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 UI] Replace IFieldType references #121108

Merged

Conversation

Kerry350
Copy link
Contributor

@Kerry350 Kerry350 commented Dec 13, 2021

Summary

Fixes #107885

Replaces all instances of IFieldType with suitable replacements.

Due to the super permissible nature of the IFieldType interface there were some types that were just represented incorrectly. But, because IFieldType designated most fields as optional it didn't really surface the type mismatches.

E.g. on the metrics side the derived index pattern was often designated as a DataViewBase, and in many ways it looks like one, the problem with the base type is it also expects the fields to be of the base version. However, we actually use a custom type for the representation there. It's not quite the "full" field, nor the base type, which is why it made more sense to introduce a new accurate type.

Over on the logs side it may well make more sense for some sections of code to use ResolvedLogSourceConfiguration['fields'] rather than relying on the underlying, backing types. However, I had issues getting that to play nicely where we use supportedFields in the alerting code (even though there should have been no type mismatch), so instead kept this as the DataViewField type.

@Kerry350 Kerry350 changed the title Remove IFieldType [Infra UI] Replace IFieldType references Dec 13, 2021
@Kerry350 Kerry350 force-pushed the 107885-replace-ifieldtype-references branch from d4f4d50 to a4b90e4 Compare December 14, 2021 13:27
@Kerry350 Kerry350 force-pushed the 107885-replace-ifieldtype-references branch from a4b90e4 to 8b66df7 Compare December 14, 2021 15:45
@Kerry350 Kerry350 self-assigned this Dec 14, 2021
@Kerry350 Kerry350 requested a review from a team December 14, 2021 15:56
@Kerry350 Kerry350 added auto-backport Deprecated - use backport:version if exact versions are needed Feature:Logs UI Logs UI feature Feature:Metrics UI Metrics UI feature release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.0.0 v8.1.0 labels Dec 14, 2021
@Kerry350 Kerry350 marked this pull request as ready for review December 14, 2021 15:59
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
infra 917 918 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
infra 988.6KB 988.5KB -63.0B
Unknown metric groups

References to deprecated APIs

id before after diff
infra 237 13 -224

History

  • 💚 Build #13109 succeeded a4b90e41df9f16bd6ddc0b12ed7e6af54855a195
  • 💔 Build #12923 failed 83cd49df5181f335ada5f854a7823479dacccc58

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

cc @Kerry350

@weltenwort weltenwort self-requested a review December 15, 2021 10:26
Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation! Out of curiosity, what brought you to this opinion?

Over on the logs side it may well make more sense for some sections of code to use ResolvedLogSourceConfiguration['fields'] rather than relying on the underlying, backing types.

@Kerry350
Copy link
Contributor Author

@weltenwort Thanks for the review.

Out of curiosity, what brought you to this opinion?

I think it was mostly with regards to refactoring. I think when we type "our" concept (source config here) and use that, it makes it a lot easier to change things later. It can also make it a bit easier to reason about where things originate from and how they fit into things overall.

There are pros and cons though. This would arguably leak concepts and implementation details to layers where they shouldn't be.

Anyway, I wimped out regardless and kept with the DataViewField type and didn't make any changes 😅 These were mostly just things bouncing around my head as I was considering the two ideas / styles when untangling the types.

@Kerry350 Kerry350 removed the v8.0.0 label Dec 15, 2021
@Kerry350 Kerry350 merged commit e599059 into elastic:main Dec 15, 2021
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Dec 15, 2021
@kibanamachine
Copy link
Contributor

💔 Backport failed

The backport operation could not be completed due to the following error:
There are no branches to backport to. Aborting.

The backport PRs will be merged automatically after passing CI.

To backport manually run:
node scripts/backport --pr 121108

@weltenwort
Copy link
Member

Yes, that makes sense. We can introduce custom types when they start to diverge from the data view concepts.

TinLe pushed a commit to TinLe/kibana that referenced this pull request Dec 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed backport:skip This commit does not require backporting Feature:Logs UI Logs UI feature Feature:Metrics UI Metrics UI feature release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Logs UI] Replace references to IFieldType
5 participants