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

[data.fieldFormats] Remove aggs format decorator from fieldFormats.deserialize() #71967

Closed
5 tasks
Tracked by #106966
lukeelmers opened this issue Jul 15, 2020 · 1 comment · Fixed by #106755
Closed
5 tasks
Tracked by #106966
Assignees
Labels
Feature:FieldFormatters impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:medium Medium Level of Effort technical debt Improvement of the software architecture and operational architecture

Comments

@lukeelmers
Copy link
Member

As part of the preparation for making aggs available on the server, we extracted the aggs-specific logic from fieldFormats.deserialize and consolidated all of it into a single utility which decorates getFieldFormat with some special agg-specific formatter types:

// decorate getFormat to handle custom types created by aggs
const getFieldFormat = getFormatWithAggs(getFormat);

While this created a clearer separation of concerns, it still feels wrong to have field formats depending on aggs in this way.

So why can't aggs just be added to the registry like other formatters? This would be ideal as they wouldn't need special treatment by the field formats service. Currently there are two reasons we can't do this:

  1. These formatters shouldn't show up in the field formats UI as they are only meant to be applied to fields for a specific aggregation type.
  2. These formatters need to access other formatters in the registry, as they are decorating the existing formatters attached to the aggregation's field param with some additional functionality.

After some discussion with @stacey-gammon around whether the enhancements pattern could solve this problem, we agreed it is probably overkill for this particular scenario.

With that in mind, I'd propose taking the following steps to resolve this:

  • Add a flag to FieldFormat that allows you to specify a format as hidden. The idea is that formats with hidden: true would not be surfaced in the UI. This could be achieved a number of ways, but perhaps the most simple way is to make it so that these formats are only available in the registry if you get them by ID (e.g. getType('some_hidden_format')), but they would not be available when you are getting collections of formats (e.g. getFieldByType('number')).
  • Provide getType to every registered FieldFormat. This way any formatter in the registry could look up other formatters by ID at runtime, which opens up the possibility of new formatters enhancing/decorating existing ones. This could probably be provided via the fieldFormatMetaParamsDecorator.
  • Register each of the existing custom agg formatters. This includes range, date_range, ip_range, and terms. These formats would be set to hidden: true and use the injected getType to access other formats at runtime.
  • Remove the static FieldFormat.from() method. This is only used internally & by the aggs formatters. If the agg formatters exist in the registry, there is no longer a need for this method to be exposed publicly.
  • Remove fieldFormats.deserialize dependency on aggs.
@lukeelmers lukeelmers added loe:medium Medium Level of Effort technical debt Improvement of the software architecture and operational architecture Team:AppArch Feature:FieldFormatters labels Jul 15, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@lukeelmers lukeelmers self-assigned this Aug 17, 2020
@lukeelmers lukeelmers removed their assignment Dec 2, 2020
@exalate-issue-sync exalate-issue-sync bot added the impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. label Jun 16, 2021
@Dosant Dosant self-assigned this Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:FieldFormatters impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:medium Medium Level of Effort 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