-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ILM] Absolute to relative time conversion #87822
[ILM] Absolute to relative time conversion #87822
Conversation
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jloleysens , thanks a lot for working on this conversion! I appreciate addition of tests and build-in i18n support, however I think having this lib function return durations in days (Infinity
for forever, fractions for less than a day) would be easier for UI integration than i18n strings. For example, change color and icon of phase duration bottom block if duration < Infinity
@elasticmachine merge upstream |
Thanks for the review @yuliacech , I've changed the lib to also export another function that returns the millisecond values per phase. This is definitely something that will be useful for the timeline component and possibly in other places too! I also expanded the test coverage to this new function. Would you mind taking another look when you get a chance? See 5158ae9. |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this, @jloleysens ! Changes LGTM, left a couple of nits in comments, but nothing blocking 👍
|
||
const i18nTexts = { | ||
forever: i18n.translate('xpack.indexLifecycleMgmt.relativeTiming.Forever', { | ||
defaultMessage: 'Forever', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think we should use a lowercase 'forever' for both the label and the string itself
defaultMessage: 'Forever', | ||
}), | ||
lessThanADay: i18n.translate('xpack.indexLifecycleMgmt.relativeTiming.lessThanADay', { | ||
defaultMessage: 'Less than a day', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: same, maybe use 'less than a day' (because it will probably be used as part of a sentence and can be text transformed if needed)
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
* cleaning up unused types and legacy logic * added new relative age logic with unit tests * export the calculate relative timing function that returns millisecond values * added exports to index.ts file * copy update and test update Co-authored-by: Kibana Machine <[email protected]>
* cleaning up unused types and legacy logic * added new relative age logic with unit tests * export the calculate relative timing function that returns millisecond values * added exports to index.ts file * copy update and test update Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Summary
Added lib logic for converting an ILM policy's minimum age timings to relative time. For example:
Additionally, the lib code includes a function for returning these values in their millisecond form, see the test coverage for examples of how the returned values look.
How to review
The code added is just lib code, it is not consumed anywhere except for the accompanying tests. These should be read and expanded in the review to make sure that we are correctly handling absolute time conversion to relative time conversion.
The goal of this code is to give users an understanding of how long their data will be in any given phase, and by extension any given data tier.
The assumption was made to always use "day" as the unit of time for this output to simplify the copy and create uniformity in the information being conveyed.
Note
Also removed some unused, legacy logic and types in 9dfe2fc
Checklist
Delete any items that are not applicable to this PR.