Remove dependencies from index pattern field and index pattern list

[mattkime]

Summary

The index pattern fields class has the following changes

• Its no longer created using a constructor. This produced odd side effects when array methods were used. In particular, removing the IndexPattern argument revealed that the FieldList constructor was being called when filter and similar were called, producing an error. Its only created once by IndexPattern.
• IndexPattern object and onNotification are no longer provided to the creation function.

The index pattern field class has the following changes

• IndexPattern object and onNotification are no longer provided to the constructor.
• The format attribute no longer exists. Use IndexPattern.getFormatterForField instead.
• No longer uses a callback when an unknown field type is encountered, instead it throws FieldTypeUnknownError
• toSpec now takes an optional argument, { getFormatterForField } which takes the field as an argument and returns a formatter.

Checklist

• Unit or functional tests were updated or added to match the most common scenarios

Dev docs

The index pattern fields class has the following changes

• Its no longer created using a constructor. This produced odd side effects when array methods were used. In particular, removing the IndexPattern argument revealed that the FieldList constructor was being called when filter and similar were called, producing an error. Its only created once by IndexPattern.
• IndexPattern object and onNotification are no longer provided to the creation function.

The index pattern field class has the following changes

• IndexPattern object and onNotification are no longer provided to the constructor.
• The format attribute no longer exists. Use IndexPattern.getFormatterForField instead.
• No longer uses a callback when an unknown field type is encountered, instead it throws FieldTypeUnknownError
• toSpec now takes an optional argument, { getFormatterForField } which takes the field as an argument and returns a formatter.

[mattkime]

fixes scope on this reference

[kindsun]

Thanks for the updates! Maps changes lgtm!

• code review
• tested locally in chrome

[walterra]

LGTM - Tested for both data grid view in transform wizard and anomaly table in single metric viewer.

[kertal]

Code LGTM, KibanaApp team code review, checked out, tested Discover and Input Controls in Chrome/Firefox, works and also fixes the 'unknown field' bug in Discover (#75025).

[patrykkopycinski]

SIEM changes look great! Thank you @mattkime 💪

[lizozom]

// nit Maybe getFieldFormatter is a more consistent name?

I'm also not sure I understand why it was necessary to remove the indexPattern from the constructor?
If there's a good reason for this, I think a comment both on the constructor of this class and on the toSpec function would be appropriate.

[ppisljar]

i think now it would be really beneficial to move format to field and get rid of the format map. i guess we could still use the format map to keep the instances of field formats around if we are worried about creating to many instances (each time someone requests it), but i would put the serialized version on the field directly.

[ppisljar]

lets add the format to the Field (non deserialized, SerializedFieldFormat)

[ppisljar]

and lets get rid of the fieldFormatMap

[mattkime]

Luckily the field formatter registry already memoizes the creation of field formatter instances so I don't think we need to worry about performance concerns.

[mattkime]

We can't really get rid of the fieldFormatMap because thats how field formats are persisted. Because we wish to load the field list dynamically, we need to save the fieldFormat info separately from the field list. There are also cases where we'd like to be able to save fieldFormat info without the field existing.

Can you explain the desire to provide a serialized fieldFormat on the IndexPatternField class? Strikes me as an odd mix of serialized and nonserialized code.

The additional implication of providing a FieldFormat instance on a IndexPatternField instance is that the creator would need access to the FieldFormatsRegistry. Currently the index pattern has a reference. Perhaps we need indexPattern.addField. This is something I considered while working in the Fields class which is too cleverly overloaded. It would make sense to me to have the field list modification methods directly on the IndexPattern class.

[lukeelmers]

lets add the format to the Field (non deserialized, SerializedFieldFormat)

I like this idea of having the SerializedFieldFormat stored on the index pattern field directly, (but not the instance itself). IMO this creates a nice separation of concerns for downstream users; if you need the field format, you can get the serialized version from the index pattern, but must go to the field formats service and use fieldFormats.deserialize if you want the actual formatter instance.

It doesn't change the dependencies of index patterns internally (you still need access to the field formats registry), but it forces external users to be explicit about whether or not they rely on the field formatters.

const formatter = fieldFormats.deserialize(indexPattern.getFieldByName('myField').format);

The current implementation also feels a little awkward in scenarios where you only want a serialized format -- for example the changes that were made to aggs now look like this:

      const format = agg.params.field
        ? agg.aggConfigs.indexPattern.getFormatterForField(agg.params.field).toJSON()
        : { id: undefined, params: undefined };

Whereas if you provided the serialized format, it would be simpler:

      const format = agg.aggConfigs.indexPattern.getFieldByName(agg.params.field.name).format
        ?? { id: undefined, params: undefined };

Providing the serialized formats this way would also remove the need to pass getFormatterForField into that field toSpec method, which feels somewhat odd to me too (I would assume that if I have an instance which I need to serialize, I don't need to provide more dependencies to it in order to perform the serialization).

An approach like this is not without precedent -- it is how we handle things inside agg config at the moment. There's an aggConfig.toSerializedFieldFormat() which will return SerializedFieldFormat for the field you are aggregating on. This was done intentionally so that you must use fieldFormats.deserialize if you need the formatter instance, rather than getting that instance from a different service.

[mattkime]

but it forces external users to be explicit about whether or not they rely on the field formatters.

I think this specifically is worth resolving. Is there value in forcing users to be explicit about dependencies? Consider it in abstract - one situation you grab the IndexPattern service which includes field formatters, another you need to grab the Index Pattern service and the field formatter service. Which is better?

As best I understand, the cases where IndexPatterns is used without field formatting is pretty small. You'd be querying for documents but not formatting the result.

In the case where the dev needs two dependencies - how are they benefitting from this additional work? I'm having trouble finding a benefit.

In the case where a dev doesn't need formatting - is there something undesirable about its presence? I could see it affecting bundle size, but I think this is a premature optimization unless we back it up with numbers.

Side note - the IndexPattern class currently handles looking up default formatters. Needs to be accounted for but should be simple to move. Actually, the reason why the getFormatterForField is there is to get the default field formatter. Maybe this would be better addressed where the SerializedFieldFormat is being consumed rather than supplying it to all FieldSpecs

[ppisljar]

then we don't need this one here, as we already have the serialized field format on the field.

[ppisljar]

the getFormatterForField function https://github.com/elastic/kibana/pull/75185/files#diff-0b63d2b724ba854c7057cfe7ac846028R488 can then change to:

this.fieldFormats.getInstance(field.format.id, field.format.params)

[mattkime]

the link isn't opening to a line of code so I'm not sure I understand.

[ppisljar]

i was trying to link to the getFormatterForField method on the index pattern class.

[mattkime]

@elasticmachine merge upstream

[ppisljar]

i don't see any of the above comments as blockers for this PR so approving this.

[mattkime]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: bd9923f

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
data	546	+1	545

async chunks size

id	value	diff	baseline
discover	430.8KB	+173.0B	430.6KB
indexPatternManagement	806.5KB	-59.0B	806.6KB
maps	3.3MB	+71.0B	3.3MB
total		+185.0B

page load bundle size

id	value	diff	baseline
data	1.4MB	+5.4KB	1.4MB
inputControlVis	295.6KB	-54.0B	295.7KB
ml	576.8KB	+64.0B	576.7KB
total		+5.4KB

oss distributable file count

id	value	diff	baseline
total	27308	+1	27307

distributable file count

id	value	diff	baseline
total	45536	+1	45535

History

• 💔 Build #71855 failed b6c1b11
• 💚 Build #71433 succeeded 97102c6
• 💚 Build #70929 succeeded 0bf4be3
• 💚 Build #70251 succeeded 4d73bf9
• 💚 Build #70127 succeeded e80efb3

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