[data.search.aggs] Remove fieldFormats from AggConfig & AggConfigs #69762
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lukeelmers
added
review
Feature:Aggregations
lukeelmers
commented
Jun 23, 2020
Comment on lines
+56
to
+59
| if (Object.keys(format).length < 1) { | ||
| // If no format exists, fall back to string as a default | ||
| format = { id: 'string' }; | ||
| } |
There was a problem hiding this comment.
This fallback behavior existed previously in agg.fieldFormatter() so we need to replicate it here or empty serialized formats ({}) don't receive any formatting, like - for an empty value.
Comment on lines
+24
to
+25
| /** @internal */ | ||
| export const createFilterHistogram = (getInternalStartServices: GetInternalStartServicesFn) => { |
There was a problem hiding this comment.
Now that we don't have aggConfig.fieldFormatter, these need to be wrapped in a provider function so that getInternalStartServices can be passed in to access fieldFormats.deserialize instead. (This is also one of the reasons agg types still depend on field formats, even though agg configs & agg config doesn't)
| expect(filter).toHaveProperty('range'); | ||
| expect(filter).toHaveProperty('meta'); | ||
| expect(filter.meta).toHaveProperty('index', '1234'); | ||
| expect(filter.range).toHaveProperty('bytes'); | ||
| expect(filter.range.bytes).toHaveProperty('gte', 1024.0); | ||
| expect(filter.range.bytes).toHaveProperty('lt', 2048.0); | ||
| expect(filter.meta).toHaveProperty('formattedValue', '≥ 1,024 and < 2,048'); | ||
| expect(filter.meta).toHaveProperty('formattedValue'); |
There was a problem hiding this comment.
We are no longer testing for specific values here since the mocks aren't returning any real values, but I don't think they were the right thing to test here anyway. Instead I'm just making sure a formattedValue is included, and also verifying that fieldFormats.deserialize was called as expected. Whether the values are deserialized correctly is something to test in field formats.
Comment on lines
+39
to
+42
| deserialize: jest.fn().mockImplementation(() => { | ||
| const DefaultFieldFormat = FieldFormat.from(identity); | ||
| return new DefaultFieldFormat(); | ||
| }), |
There was a problem hiding this comment.
Added a mock implementation to improve these somewhat. I just stole this from the getFormat function inside of fieldFormats.deserialize.
Important to note that since this is configured with identity, any calls against this mock are just going to return whatever they are given. So you'll get a convert function you can use, but of course it won't actually convert what you give to it.
| type: { | ||
| id: 'bytes', | ||
| }, | ||
| toJSON: () => ({ id: 'bytes' }), |
There was a problem hiding this comment.
These mocks were changed because this is basically the only part of field formats that AggConfig cares about: getting the serialized format for the field from the provided index pattern
| @@ -54,6 +54,7 @@ export const getGeoHashBucketAgg = ({ getInternalStartServices }: GeoHashBucketA | |||
| { | |||
| name: BUCKET_TYPES.GEOHASH_GRID, | |||
| title: geohashGridTitle, | |||
| makeLabel: () => geohashGridTitle, | |||
There was a problem hiding this comment.
Though not directly related to the other items in this PR, I added these makeLabel methods to geo hash & filter aggs in this PR because I noticed they were missing, which causes column headers to be unformatted in the inspector.
7 tasks
lukeelmers
force-pushed
the
fix/remove-agg-formats
branch
from
864f503 to
bf8e6de
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
lukeelmers
force-pushed
the
fix/remove-agg-formats
branch
from
bf8e6de to
b035324
Compare
|
Pinging @elastic/kibana-app-arch (Team:AppArch) |
flash1293
approved these changes
Jun 25, 2020
kertal
approved these changes
Jun 25, 2020
There was a problem hiding this comment.
KibanaApp owned code looks beautiful, table viz still works (tested in Chrome) ... finishing this review even @flash1293 overtook while had to take care of child logistics 🧒 -> 🐘 :)
This comment has been minimized.
This comment has been minimized.
|
@elasticmachine merge upstream |
💚 Build SucceededBuild metricspage load asset sizebeta
History
To update your PR or re-run it, just comment with: |
lukeelmers
added a commit
that referenced
this pull request
Jun 29, 2020
Bamieh
pushed a commit
to Bamieh/kibana
that referenced
this pull request
Jul 1, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(3 of 3)
Part of #65793
Summary
In #69114 and #69586 we gave AggConfigs the ability to return a serializable representation of the field format for the particular field the agg was using. This
SerializedFieldFormatcould then be passed tofieldFormats.deserializeso that downstream users could retrieve the actual format instance.Since that feature has been introduced, there is no longer a need for an AggConfig to expose a way to retrieve the format instance for a particular field. Instead, anyone needing this should use
toSerializedFieldFormatanddeserializeas described above.This also means that AggConfig no longer needs to depend directly on the fieldFormats service. (It does technically have an implicit dependency on field formats via
indexPattern.fields.someField.format, but this has always been the case... ideally we would instead get the serialized field format off the index pattern instead).Note that each of the AggTypes still do have a dependency on fieldFormats. This is something we should ideally refactor out eventually, but wasn't feasible to do here without a significant overhaul. The main barriers to removing formats from AggType are a few places where
makeLabelandcreateFilterdepend on field formats to format the values.Items changed in this PR include:
fieldFormatterandfieldOwnFormatterfromAggConfiggetFormatfrom all of theAggTypesbuildTabularInspectorDatato retrieve the field format viatoSerializedFieldFormatanddeserializeTesting
This is simply a refactor of existing code and should introduce no functional changes. Areas to check for regressions are inspector and table vis. In the inspector, fields in the "data" tab should continue to have field formats applied as expected. In table vis, under the "options" tab, the
Percentage Columndropdown should continue to only display numerical fields.Dev Docs
AggConfighas been updated to no longer return a field format instance for the field it is aggregating on. As a result, thefieldFormatterandfieldOwnFormattermethods have been removed. Additionally, thegetFormatmethod has been removed from each individual agg type.If you need to access a field format instance, use the newly-added
AggConfig.toSerializedFieldFormat, orAggType.toSerializedFormatto retrieve the serializable representation of the field's format, and then pass it to thedeserializemethod from the field formats service to get the actual format instance.class MyPlugin { async start(core, { data }) { const { indexPatterns, fieldFormats, search } = data; const indexPattern = await indexPatterns.get('myId'); const agg = { type: 'terms', params: { field: 'machine.os.keyword' }, }; const aggConfigs = search.aggs.createAggConfigs(indexPattern, [agg]); const termsAgg = aggConfigs.aggs[0]; - const formatter = termsAgg.type.getFormat(termsAgg); - // or - const formatter = termsAgg.fieldFormatter('text'); + const formatter = fieldFormats.deserialize(termsAgg.toSerializedFieldFormat()); + // or + const formatter = fieldFormats.deserialize(termsAgg.type.getSerializedFormat(termsAgg)); const formattedValue = formatter.convert('myValue'); } }In addition, the legacy formatting helpers that were exported from
ui/visualize/loader/pipeline_helpers/utilitieshave been removed. If your plugin imports from this directory, please update your code to use thefieldFormatsservice directly.