[App Search] New log retention components + major i18n refactors #88237

cee-chen · 2021-01-13T18:56:37Z

Summary

1. (blocking) `4991c8b`

This commit migrates over LogRetentionTooltip and LogRetentionCallout from ent-search's standalone UI. It will used by the Analytics and API logs views and is blocking for future Analytics PRs that I'll be opening here shortly:

Heads up: I significantly changed how these components work compared to ent-search. Instead of having parent views/components handle passing in content, I wanted these components to manage their own visibility and content. Views calling these components should have APIs as simple as this:

<LogRetentionCallout type="analytics" />
<LogRetentionTooltip type="api" />

That's it, no need to import/check logRetention from LogRetentionLogic. This greatly simplified my Analytics work views & allowed them to remain focused on Analytics-only logic.

@byronhulcher let me know if you're cool with this approach and change. :)

2. (i18n) `2fa5534`

Because I'm very extra, I noticed that our log retention dates weren't being localized:

Switching from moment to FormattedDate allows us to correctly localize dates according to the current i18n locale:

This does however significantly mess up our typing, which was expecting strings instead of JSX/React components, which led me to the following commits...

3. (cleanup) `c8a5a44`, `00eb97a`, `8e7fd56`

These PRs streamline log_retention/messaging and simplifies our i18n handling of those messages.

Consolidate to a single React component which takes different log types (e.g. api, analytics, crawler in the future)
Handle all copy expecting JSX (vs strings), to accommodate i18n/FormattedMessage/FormattedDate
Consolidate our i18n strings - the current map system we have in ent-search unfortunately does not scale well with i18n in Kibana. Considering that a team is translating each string we're creating and how alike these messages are, it makes far more sense to DRY out each message to accept a different log type so that adding new log types only adds 2 new very short strings each time instead of 5+ longer & complex sentences each.

For the most part, this should behave as before with some minor copy changes/standardization (e.g., analytics collection -> analytics logging):

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios

- Will shortly be used by Analytics view

- this correctly localizes the dates when Kibana is in different languages, e.g. zh-CN, ja-JP

- Convert to all JSX/React vs plain strings (makes it easier to deal with FormattedDate/FormattedMessages) - Consolidate to single component that takes various log types (e.g. analytics, api) - This is important for adding future switches & toggles - there is *significantly* less complex strings to localize this way (2 short strings vs 5+ long sentences)

- to account for new i18n logs type values & IDs

byronhulcher · 2021-01-13T20:28:23Z

...public/applications/app_search/components/log_retention/components/log_retention_callout.tsx

+  const logRetentionSettings = logRetention?.[type];
+  const title = TITLE_MAP[type];
+
+  const hasILM = logRetention !== null;


I know ILM is an Elastic acronym, but its weird that we refer to this as "Log Retention" everywhere except for a few boolean values. Worth dropping the acronym internally? hasLogRetention hasLogRetentionDisabled?

I have no strong preference, I think my only hesitation is there are some instances where ILM does actually reference to separate Elasticsearch concept/architecture that can be disabled separately, whereas Enterprise Search can override ILM (mostly by disabling it), at which point there's the broader concept of "Log Retention" separate from ILM.

I think I'd slightly prefer to leave it as-is unless we can check with Chris/back-end engineers to make sure our understanding is correct?

I ended up liking this way more over time and updated the tooltip & callout components to use your suggestion in a448f12 - thanks Byron!!

byronhulcher · 2021-01-13T20:31:55Z

...prise_search/public/applications/app_search/components/log_retention/messaging/constants.tsx

+  disabledAt?: string | null;
+  minAgeDays?: number | null;


Raises an eyebrow to me that these can be both undefined and are nullable, two different ways of representing when a value is "missing". If these represent distinct states, we might be doing too much.

🤷 | null is the current definition in ent-search, I think we just copied it over. Looks like you originally defined this 6 months ago there? I have no strong feelings either way, this comes in from the server so it would basically just be confirming that the server can or cannot serve a null

Oh if you're raising your eyebrow at the undefined / ?, I mean that's because we're passing around optional props.

That's probably easily solvable by doing minAgeDays?: LogRetentionPolicy[minAgeDays]. I can definitely make that change.

byronhulcher · 2021-01-13T20:32:24Z

...prise_search/public/applications/app_search/components/log_retention/messaging/constants.tsx

+export const DefaultPolicy: React.FC<Props> = ({ type, minAgeDays }) => (
+  <FormattedMessage
+    id="xpack.enterpriseSearch.appSearch.logRetention.defaultPolicy"
+    defaultMessage="Your {logsType} logs are being stored for at least {minAgeDays} days."


does 1 days matter here?

Ah great catch!! I'll add some i18n pluralization here

byronhulcher · 2021-01-13T20:34:12Z

x-pack/plugins/translations/translations/ja-JP.json

@@ -7279,20 +7279,18 @@
    "xpack.enterpriseSearch.appSearch.productName": "App Search",
    "xpack.enterpriseSearch.appSearch.result.hideAdditionalFields": "追加フィールドを非表示",
    "xpack.enterpriseSearch.appSearch.roleMappings.title": "ロールマッピング",
-    "xpack.enterpriseSearch.appSearch.settings.logRetention.analytics.customPolicy": "カスタム分析保持ポリシーがあります。",
-    "xpack.enterpriseSearch.appSearch.settings.logRetention.analytics.ilmDisabled": "App Search は分析保持を管理していません。",
+    "xpack.enterpriseSearch.appSearch.logRetention.customPolicy": "カスタム {logsType} ログ保持ポリシーがあります。",


Just curious, where are these translations coming from?

There's an i18n team+contractors that I think check for untranslated IDs every release? I'm not super clear myself lol, the #kibana-localization channel might know. Also https://github.com/elastic/kibana/tree/master/packages/kbn-i18n for reference

cee-chen · 2021-01-14T18:18:31Z

FYI - I'm adding another commit to this PR that has <LogRetentionCallout /> and <LogRetentionTooltip /> also handle fetchLogRetention themselves. I only just realized I was calling fetchLogRetention from the Analytics router page load to populate the callout/tooltip, and figured the components could be intelligently & modularly handling everything log-retention-related instead of relying on their parents view.

… log retention + change ILM to LogRetention per PR feedback

- e.g. if both LogRetentionTooltip and LogRetentionCallout are on the same page (which they will be)

cee-chen · 2021-01-14T18:55:57Z

@JasonStoltz If you have time today, do you mind taking another super quick glance at the 2 new commits and re-approving if they look sane?

kibanamachine · 2021-01-14T20:47:54Z

💚 Build Succeeded

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

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`enterpriseSearch`	1076	1079	+3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`enterpriseSearch`	1.8MB	1.8MB	+5.3KB

History

💚 Build #98744 succeeded 1829250
💚 Build #98682 succeeded b56397a
💔 Build #98656 failed 8e7fd56

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

JasonStoltz · 2021-01-14T21:56:37Z

Looks fine to me @constancecchen

cee-chen · 2021-01-15T00:00:59Z

Thanks a million!

) (#88421) * Add remaining log retention components - Will shortly be used by Analytics view * [i18n] Change log retention date from moment to react-intl FormattedDate - this correctly localizes the dates when Kibana is in different languages, e.g. zh-CN, ja-JP * Refactor log_retention/messaging - Convert to all JSX/React vs plain strings (makes it easier to deal with FormattedDate/FormattedMessages) - Consolidate to single component that takes various log types (e.g. analytics, api) - This is important for adding future switches & toggles - there is *significantly* less complex strings to localize this way (2 short strings vs 5+ long sentences) * Update existing instances to use new LogRetentionMessage * Update translation strings - to account for new i18n logs type values & IDs * Attempt to fix test timezone shenanigans * [PR feedback] Types * [PR feedback] i18n pluralization * Update LogRetentionTooltip and LogRetentionCallout to manage fetching log retention + change ILM to LogRetention per PR feedback * Update LogRetentionLogic to prevent duplicate fetches - e.g. if both LogRetentionTooltip and LogRetentionCallout are on the same page (which they will be)

cee-chen added 5 commits January 13, 2021 08:28

Add remaining log retention components

4991c8b

- Will shortly be used by Analytics view

[i18n] Change log retention date from moment to react-intl FormattedDate

2fa5534

- this correctly localizes the dates when Kibana is in different languages, e.g. zh-CN, ja-JP

Update existing instances to use new LogRetentionMessage

00eb97a

Update translation strings

8e7fd56

- to account for new i18n logs type values & IDs

cee-chen added release_note:skip Skip the PR/issue when compiling release notes v7.12.0 labels Jan 13, 2021

cee-chen requested review from a team, JasonStoltz and byronhulcher January 13, 2021 18:56

Attempt to fix test timezone shenanigans

b56397a

byronhulcher reviewed Jan 13, 2021

View reviewed changes

cee-chen added 2 commits January 13, 2021 15:21

[PR feedback] Types

390ace7

[PR feedback] i18n pluralization

1829250

JasonStoltz approved these changes Jan 14, 2021

View reviewed changes

cee-chen added 3 commits January 14, 2021 10:27

Merge remote-tracking branch 'upstream/master' into log-retention-2

1a47c90

Update LogRetentionTooltip and LogRetentionCallout to manage fetching…

a448f12

… log retention + change ILM to LogRetention per PR feedback

Update LogRetentionLogic to prevent duplicate fetches

c1978e5

- e.g. if both LogRetentionTooltip and LogRetentionCallout are on the same page (which they will be)

JasonStoltz approved these changes Jan 14, 2021

View reviewed changes

cee-chen merged commit ca42e9a into elastic:master Jan 15, 2021

cee-chen deleted the log-retention-2 branch January 15, 2021 00:01

cee-chen mentioned this pull request Jan 15, 2021

[7.x] [App Search] New log retention components + major i18n refactors (#88237) #88421

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[App Search] New log retention components + major i18n refactors #88237

[App Search] New log retention components + major i18n refactors #88237

cee-chen commented Jan 13, 2021 •

edited

Loading

byronhulcher Jan 13, 2021

cee-chen Jan 13, 2021

cee-chen Jan 14, 2021

byronhulcher Jan 13, 2021

cee-chen Jan 13, 2021 •

edited

Loading

cee-chen Jan 13, 2021 •

edited

Loading

cee-chen Jan 13, 2021

byronhulcher Jan 13, 2021

cee-chen Jan 13, 2021

cee-chen Jan 13, 2021

byronhulcher Jan 13, 2021

cee-chen Jan 13, 2021 •

edited

Loading

cee-chen commented Jan 14, 2021

cee-chen commented Jan 14, 2021

kibanamachine commented Jan 14, 2021

JasonStoltz commented Jan 14, 2021 •

edited

Loading

cee-chen commented Jan 15, 2021

[App Search] New log retention components + major i18n refactors #88237

[App Search] New log retention components + major i18n refactors #88237

Conversation

cee-chen commented Jan 13, 2021 • edited Loading

Summary

1. (blocking) 4991c8b

2. (i18n) 2fa5534

3. (cleanup) c8a5a44, 00eb97a, 8e7fd56

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

cee-chen Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

cee-chen commented Jan 14, 2021

cee-chen commented Jan 14, 2021

kibanamachine commented Jan 14, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

JasonStoltz commented Jan 14, 2021 • edited Loading

cee-chen commented Jan 15, 2021

cee-chen commented Jan 13, 2021 •

edited

Loading

1. (blocking) `4991c8b`

2. (i18n) `2fa5534`

3. (cleanup) `c8a5a44`, `00eb97a`, `8e7fd56`

cee-chen Jan 13, 2021 •

edited

Loading

cee-chen Jan 13, 2021 •

edited

Loading

cee-chen Jan 13, 2021 •

edited

Loading

JasonStoltz commented Jan 14, 2021 •

edited

Loading