[data.search.aggs] Remove fieldFormats from AggConfig & AggConfigs #69620
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 19, 2020
| const value = parseInt(key, 10); | ||
| const params: RangeFilterParams = { gte: value, lt: value + aggConfig.params.interval }; | ||
| /** @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 doesnt)
| 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
|
@elasticmachine merge upstream |
💔 Build Failed
Failed CI StepsTest FailuresKibana Pipeline / kibana-oss-agent / Chrome UI Functional Tests.test/functional/apps/visualize/_tile_map·js.visualize app tile map visualize app complete config tile map chart should show correct tile map data on default zoom levelStandard OutStack TraceKibana Pipeline / kibana-oss-agent / Chrome UI Functional Tests.test/functional/apps/visualize/_tile_map·js.visualize app tile map visualize app complete config tile map chart should show correct tile map data on default zoom levelStandard OutStack TraceBuild metrics@kbn/optimizer bundle module count
History
To update your PR or re-run it, just comment with: |
7 tasks
* Fix flaky endpoints list unit test * un-skip test Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* Change the bootstrap of the app * rename SiemPageName to SecurityPageName * modify alerts routes * modify cases routes * modify hosts routes * modify network routes * modify overview routes * modify timelines routes * wip change management route * change route for common * some fixing from the first commit * modify route for management * update url format hook to use history * bug when you click on external alerts from host or network * improvement from josh feedback * redirect siem to security solution * a little clean up * Fix types * fix breadcrumbs * fix unit test * Update index.tsx * Fix cypress * bug remove timeline when you are in case configure * Fix functionel test for management * Fix redirect siem + ml * fixes some cypress tests * adds 'URL compatibility' test * bring ml back to alerts * review I * Fix memory leak in timelines page * fix storage bug for timeline search bar * fix endpoint merge + functional test * avoid timeline flyout toggle * Fix link to ml score * Fix breadcrumb * Fix management url * fix unit test * fixes typecheck issue * fixes remaining url cypress tests * fixes timeline scenario * fix link to details rule from timeline * review remove absolute path for consistency * Fixing resolver alert generation (elastic#69587) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> * [Security_Solution][Endpoint] Resolver leverage ancestry array for queries (elastic#69264) * Adding alerts route * Adding related alerts generator changes, tests, and script updates * Fixing missed parameter * Aligning the AlertEvent and ResolverEvent definition * Fixing type errors * Fixing import error * Adding ancestry functionality in generator * Creating some tests for ancestry field * Making progress on the ancestry * Fixing the ancestry verification * Fixing existing tests * Removing unused code and fixing test * Adding more comments * Fixing endgame queries Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> * fix cypress test * skip failing suite (elastic#69595) * [Endpoint] Fix flaky endpoints list unit test (elastic#69591) * Fix flaky endpoints list unit test * un-skip test Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> * remove flaky test Co-authored-by: patrykkopycinski <contact@patrykkopycinski.com> Co-authored-by: Gloria Hornero <snootchie.boochies@gmail.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Jonathan Buttner <56361221+jonathan-buttner@users.noreply.github.com> Co-authored-by: spalger <spalger@users.noreply.github.com> Co-authored-by: Paul Tavares <56442535+paul-tavares@users.noreply.github.com>
Closes: elastic#67469 Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
…9257) * Adding specific apis for each plugin * adding metric hosts stat * addressing PR comment * addressing PR comments * changing series to key/value * exporting interfaces * adding label to stat * refactoring types Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
lukeelmers
force-pushed
the
fix/aggs-remove-formats
branch
from
af8f1c3
to
2087dde
Compare
|
Closing in favor of #69762 |
|
Can you please add a Dev Doc section in the PR Summary? Release notes pull the summary of the PR from the Dev Doc section. |
lukeelmers
added
release_note:skip
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.
Closed in favor of #69762