Expose NP FieldFormats service to server side #55419

alexwizp · 2020-01-21T16:06:41Z

Summary

The fieldFormats service is used on the server side as well.
At the moment, it resides in src/legacy/ui/field_formats/mixin/field_formats_service.ts and only the FE plugin exposes it.

We should expose the field registry from the server side plugin at data.fieldFormats.

Dev Docs

The fieldFormats service is used on the server side as well.
At the moment, it resides in src/legacy/ui/field_formats/mixin/field_formats_service.ts and only the FE plugin exposes it.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2020-01-22T12:11:30Z

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

alexwizp · 2020-01-22T12:18:52Z

@elasticmachine merge upstream

alexwizp · 2020-01-22T12:35:54Z

.eslintrc.js

@@ -244,15 +244,15 @@ module.exports = {
              {
                target: [
                  '(src|x-pack)/plugins/**/*',
-                  '!(src|x-pack)/plugins/*/server/**/*',


modified to add support for:
x-pack/legacy/plugins/reporting/export_types/csv/server/

# Conflicts: # src/legacy/core_plugins/kibana/index.js # src/legacy/ui/ui_mixin.js

src/plugins/data/server/field_formats/field_formats_service.ts

flash1293

Kibana app changes lgtm - however it would be nice if we would export FieldFormat separately from the top level so we don't have to pull it out of the helper. Not a blocker though

# Conflicts: # src/legacy/ui/public/agg_types/agg_config.ts # src/legacy/ui/public/agg_types/agg_type.ts # src/legacy/ui/public/agg_types/buckets/terms.ts # src/plugins/data/public/search/search_source/search_source.ts

lizozom

As far as I'm concerned, I'm ok with merging this.
We can figure out the exact interface of field formats on the server side with @elastic/kibana-platform and do a follow up.

@lukeelmers what do you think?

alexwizp · 2020-01-27T15:12:48Z

@elasticmachine merge upstream

lukeelmers

LGTM! Thanks for dealing with all of the merge conflicts as we were reviewing @alexwizp 😉

lukeelmers · 2020-01-27T19:04:25Z

src/plugins/data/server/field_formats/field_formats_service.ts

+/** @public */
+export type FieldFormatsSetup = ReturnType<FieldFormatsService['setup']>;
+
+/** @public */
+export type FieldFormatsStart = ReturnType<FieldFormatsService['start']>;


Ah, yes I misread that on the server the returned fieldFormatsRegistry is wrapped in the service factory. I thought they actually returned the same values which was why I commented in the first place.

In this case I agree with @lizozom, I don't want to block this PR on resolving it, so let's revisit later.

For now @alexwizp would you please add a TODO on the server service code noting that we should come back and remove the factory so server/client can have the same interface?

lukeelmers · 2020-01-27T19:11:53Z

Just a note that I'm adding the dev docs label to this PR so that it shows up in our tooling as "missing docs" -- that way we don't forget to make the necessary updates as we approach release.

eliperelman

LGTM for migration.md

* Expose NP FieldFormats service to server side * fix CI * fix PR comments * fix PR comments * fix CI * getFieldFormatsRegistry -> getFieldFormatRegistry * fix CI * memoize - add resolve cache function * fix Jest * move IFieldFormatMetaParams to types.ts * FieldFormatRegistry -> FieldFormatsRegistry * update src/core/MIGRATION.md * update public contract Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> # Conflicts: # x-pack/legacy/plugins/reporting/export_types/csv/server/execute_job.test.js

* Expose NP FieldFormats service to server side (#55419) * Expose NP FieldFormats service to server side * fix CI * fix PR comments * fix PR comments * fix CI * getFieldFormatsRegistry -> getFieldFormatRegistry * fix CI * memoize - add resolve cache function * fix Jest * move IFieldFormatMetaParams to types.ts * FieldFormatRegistry -> FieldFormatsRegistry * update src/core/MIGRATION.md * update public contract Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> # Conflicts: # x-pack/legacy/plugins/reporting/export_types/csv/server/execute_job.test.js * fix merge conflict

…ve-out-legacy * 'master' of github.com:elastic/kibana: (187 commits) [ML] Reseting categorization validation if category field is cleared (elastic#56029) [SIEM] Fields browser readable (elastic#56000) [docs] Remove unused callout (elastic#56032) Refactor saved object management registry usage (elastic#54155) [SIEM][Detection Engine] critical blocker, updates the pre-packaged rules, removes dead ones, adds license file (elastic#56090) Fix failing snapshot artifact tests when using env var (elastic#56063) Fix Github PR comment formatting (elastic#56078) [Maps] fix join metric field selection bugs (elastic#56044) Create a new menu for observability links (elastic#54847) [SIEM] [Detection Engine] Fixes histogram intervals (elastic#55969) make test less flaky by retrying if list is re-rendered (elastic#55949) Remove matrix build support (elastic#54202) Add animation to service map layout (elastic#56042) [Canvas] Remove Angular and unnecessary reporting config from Canvas (elastic#54050) [Uptime] Simplify snapshot max to Infinity (elastic#55931) [Uptime] Reintroduce a column for url (elastic#55451) Cleanup action task params objects after successful execution (elastic#55227) [CI] Retry flaky tests (elastic#53961) Expose NP FieldFormats service to server side (elastic#55419) [Endpoint] EMT-65: make endpoint data types common, restructure (elastic#54772) ... # Conflicts: # src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/__snapshots__/split_panel.test.tsx.snap # src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/containers/panel.tsx # src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/context.tsx # src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/index.ts # src/legacy/core_plugins/console/public/np_ready/application/components/split_panel/split_panel.test.tsx # src/legacy/ui/public/vis/editors/default/default_editor.tsx # src/plugins/console/public/application/components/split_panel/__snapshots__/split_panel.test.tsx.snap # src/plugins/console/public/application/components/split_panel/components/resizer.tsx # src/plugins/console/public/application/components/split_panel/containers/panel.tsx # src/plugins/console/public/application/components/split_panel/containers/panel_container.tsx # src/plugins/console/public/application/components/split_panel/context.tsx # src/plugins/console/public/application/components/split_panel/index.ts # src/plugins/console/public/application/components/split_panel/registry.ts # src/plugins/console/public/application/components/split_panel/split_panel.test.tsx # src/plugins/kibana_react/public/split_panel/__snapshots__/split_panel.test.tsx.snap # src/plugins/kibana_react/public/split_panel/containers/panel.tsx # src/plugins/kibana_react/public/split_panel/context.tsx # src/plugins/kibana_react/public/split_panel/index.ts # src/plugins/kibana_react/public/split_panel/split_panel.test.tsx

* master: (77 commits) [ML] Reseting categorization validation if category field is cleared (elastic#56029) [SIEM] Fields browser readable (elastic#56000) [docs] Remove unused callout (elastic#56032) Refactor saved object management registry usage (elastic#54155) [SIEM][Detection Engine] critical blocker, updates the pre-packaged rules, removes dead ones, adds license file (elastic#56090) Fix failing snapshot artifact tests when using env var (elastic#56063) Fix Github PR comment formatting (elastic#56078) [Maps] fix join metric field selection bugs (elastic#56044) Create a new menu for observability links (elastic#54847) [SIEM] [Detection Engine] Fixes histogram intervals (elastic#55969) make test less flaky by retrying if list is re-rendered (elastic#55949) Remove matrix build support (elastic#54202) Add animation to service map layout (elastic#56042) [Canvas] Remove Angular and unnecessary reporting config from Canvas (elastic#54050) [Uptime] Simplify snapshot max to Infinity (elastic#55931) [Uptime] Reintroduce a column for url (elastic#55451) Cleanup action task params objects after successful execution (elastic#55227) [CI] Retry flaky tests (elastic#53961) Expose NP FieldFormats service to server side (elastic#55419) [Endpoint] EMT-65: make endpoint data types common, restructure (elastic#54772) ...

tsullivan · 2020-01-28T21:17:47Z

x-pack/legacy/plugins/reporting/index.ts

@@ -70,6 +77,14 @@ export const reporting = (kibana: any) => {
      const pluginsSetup: ReportingSetupDeps = {
        usageCollection: server.newPlatform.setup.plugins.usageCollection,
      };
+
+      const fieldFormatServiceFactory = async (uiSettings: IUiSettingsClient) => {
+        const [, plugins] = await coreSetup.getStartServices();


Hi, what does this do? This is not the only core service that we use. Why is it only necessary to implement this one using a callback?

I talked to @alexwizp and we came to an understanding that this belongs in the existing ReportingPlugin#start method: https://github.com/elastic/kibana/blob/f0fbb99/x-pack/legacy/plugins/reporting/server/plugin.ts#L114

@alexwizp will make that change, so the Reporting team does not absorb the added tech debt and can keep focus on our other migration tasks.

tsullivan · 2020-01-28T21:18:45Z

x-pack/legacy/plugins/reporting/index.ts

 const kbToBase64Length = (kb: number) => {
  return Math.floor((kb * 1024 * 8) / 6);
 };

+interface ReportingDeps {
+  data: DataPluginStart;
+}


Why do we need this interface? It seems redundant with ReportingSetupDeps

tsullivan · 2020-01-28T21:21:21Z

@alexwizp @lukeelmers I have some questions about the changes to the Reporting code. When we look at how other dependencies are being pulled in for Reporting migration, this one really stands out.

kibanamachine · 2020-04-10T16:55:05Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 5072086

History

💚 Build #22762 succeeded 5072086

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

alexwizp force-pushed the 54571 branch 2 times, most recently from 09ee28a to d6cb97f Compare January 22, 2020 08:37

Expose NP FieldFormats service to server side

08c845d

alexwizp force-pushed the 54571 branch from d6cb97f to 44b73dc Compare January 22, 2020 10:46

fix CI

7752b6e

alexwizp force-pushed the 54571 branch from 44b73dc to 7752b6e Compare January 22, 2020 10:50

elastic deleted a comment from kibanamachine Jan 22, 2020

alexwizp requested review from lukeelmers, lizozom and ppisljar January 22, 2020 12:10

alexwizp self-assigned this Jan 22, 2020

alexwizp added v7.7.0 v8.0.0 Feature:FieldFormatters release_note:skip Skip the PR/issue when compiling release notes labels Jan 22, 2020

alexwizp added this to In progress in kibana-app-arch via automation Jan 22, 2020

alexwizp added the Team:AppArch label Jan 22, 2020

alexwizp added the Feature:NP Migration label Jan 22, 2020

alexwizp marked this pull request as ready for review January 22, 2020 12:11

alexwizp requested a review from a team as a code owner January 22, 2020 12:11

Merge branch 'master' into 54571

02df835

alexwizp commented Jan 22, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into 54571

e86b069

# Conflicts: # src/legacy/core_plugins/kibana/index.js # src/legacy/ui/ui_mixin.js

lizozom reviewed Jan 22, 2020

View reviewed changes

src/plugins/data/server/field_formats/field_formats_service.ts Show resolved Hide resolved

alexwizp changed the title ~~Expose NP FieldFormats service to server side~~ [WIP] Expose NP FieldFormats service to server side Jan 22, 2020

alexwizp added the WIP Work in progress label Jan 22, 2020

alexwizp added 2 commits January 23, 2020 12:20

fix PR comments

0be4619

Merge remote-tracking branch 'upstream/master' into 54571

b763a66

alexwizp requested a review from a team as a code owner January 27, 2020 08:46

update public contract

74d5735

flash1293 approved these changes Jan 27, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into 54571

b205ba6

# Conflicts: # src/legacy/ui/public/agg_types/agg_config.ts # src/legacy/ui/public/agg_types/agg_type.ts # src/legacy/ui/public/agg_types/buckets/terms.ts # src/plugins/data/public/search/search_source/search_source.ts

lizozom approved these changes Jan 27, 2020

View reviewed changes

elasticmachine and others added 3 commits January 27, 2020 10:12

Merge branch 'master' into 54571

523d4b5

Merge remote-tracking branch 'upstream/master' into 54571

7ff4c2d

Merge remote-tracking branch 'upstream/master' into 54571

5072086

elastic deleted a comment from kibanamachine Jan 27, 2020

lukeelmers approved these changes Jan 27, 2020

View reviewed changes

lukeelmers added release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. and removed release_note:skip Skip the PR/issue when compiling release notes WIP Work in progress labels Jan 27, 2020

eliperelman approved these changes Jan 27, 2020

View reviewed changes

alexwizp merged commit 5d6dbf0 into elastic:master Jan 27, 2020

kibana-app-arch automation moved this from Review in progress to Done in 7.7 Jan 27, 2020

alexwizp mentioned this pull request Jan 27, 2020

[7.x] Expose NP FieldFormats service to server side (#55419) #56052

Merged

tsullivan reviewed Jan 28, 2020

View reviewed changes

tsullivan mentioned this pull request Jan 30, 2020

[Reporting] New Platform Migration #53898

Closed

19 tasks

lukeelmers moved this from Done in current release to Done in previous release in kibana-app-arch Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose NP FieldFormats service to server side #55419

Expose NP FieldFormats service to server side #55419

alexwizp commented Jan 21, 2020 •

edited by jinmu03

Loading

elasticmachine commented Jan 22, 2020

alexwizp commented Jan 22, 2020

alexwizp Jan 22, 2020

flash1293 left a comment

lizozom left a comment

alexwizp commented Jan 27, 2020

lukeelmers left a comment

lukeelmers Jan 27, 2020

lukeelmers commented Jan 27, 2020

eliperelman left a comment

tsullivan Jan 28, 2020

tsullivan Jan 29, 2020

tsullivan Jan 28, 2020

tsullivan commented Jan 28, 2020

kibanamachine commented Apr 10, 2020

Expose NP FieldFormats service to server side #55419

Expose NP FieldFormats service to server side #55419

Conversation

alexwizp commented Jan 21, 2020 • edited by jinmu03 Loading

Summary

Dev Docs

Checklist

For maintainers

elasticmachine commented Jan 22, 2020

alexwizp commented Jan 22, 2020

alexwizp Jan 22, 2020

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

lizozom left a comment

Choose a reason for hiding this comment

alexwizp commented Jan 27, 2020

lukeelmers left a comment

Choose a reason for hiding this comment

lukeelmers Jan 27, 2020

Choose a reason for hiding this comment

lukeelmers commented Jan 27, 2020

eliperelman left a comment

Choose a reason for hiding this comment

tsullivan Jan 28, 2020

Choose a reason for hiding this comment

tsullivan Jan 29, 2020

Choose a reason for hiding this comment

tsullivan Jan 28, 2020

Choose a reason for hiding this comment

tsullivan commented Jan 28, 2020

kibanamachine commented Apr 10, 2020

💔 Build Failed

History

alexwizp commented Jan 21, 2020 •

edited by jinmu03

Loading