[Enterprise Search] Log retention settings logic #82364

JasonStoltz · 2020-11-02T19:51:00Z

Summary

This PR migrates the Logic required for the Log Retention Settings from the old Enterprise Search front-end to Kibana.

This PR is put together in a sequence of commits that each add a single logic action. It is best reviewed commit by commit.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

Note that I changd it to set logsRetentionUpdating to false instead of null so that our types were correct

cee-chen

This looks amazing!! I have a spicy proposal for you but I should hopefully have done most of the legwork on it (first comment). Would love to hear your thoughts!

cee-chen · 2020-11-04T03:06:43Z

...lic/applications/app_search/components/settings/log_retention/utils/convert_log_retention.ts

+  ILogRetentionSettings,
+} from '../types';
+
+export const convertLogRetentionFromServerToClient = (


I think we have a really unique opportunity in Kibana to clean up a lot of these ServerToClient transforms/conversions by handling them in our server/API code rather than our public/client-side code. We do something very similar already in our internal client API call, for example.

I played around with this a bit and have a working diff for you:

JasonStoltz/kibana@log-retention-logic...constancecchen:server-json-transform

Here's some screenshots of the final API output coming from Kibana:

Would really love to hear what you think! Personally, I think it really helps separate concerns quite a bit.

In any other system, I would be in favor of moving stuff like this to the server.

In the case of Kibana, I am not in favor of it. I am strongly in favor of keeping our Kibana server API as thin as possible as primarily as pass through to Enterprise Search. It feels like we're monkey patching the Enterprise Search API rather than fixing it at its source.

It feels like we're monkey patching the Enterprise Search API rather than fixing it at its source.

Yes, but we're doing that anyway with these ServerToClient utilities. I'm simply proposing moving the currently existing logic to the server/ folder.

I am strongly in favor of keeping our Kibana server API as thin as possible as primarily as pass through to Enterprise Search.

I don't particularly see why our server API needs to be this way. This is also still a relatively thin action, IMO. I'm not sure what advantage we gain with that mindset and I think being overly prescriptive about what we do in our server/ code is a wasted opportunity for cleanup / giving more freedom & control to our front-end devs.

I don't particularly see why our server API needs to be this way. This is also still a relatively thin action, IMO. I'm not sure what advantage we gain with that mindset and I think being overly prescriptive about what we do in our server/ code is a wasted opportunity for cleanup / giving more freedom & control to our front-end devs.

It's just one less place we have to look for logic. One less architectural piece to account for.

Also, if we just keep things as is, it makes it easier to just port code over from the old repository. This is one more step for porting views that we could easily cut out.

It's just one less place we have to look for logic. One less architectural piece to account for.

I'm not sold by that argument because convertLogRetentionFromServerToClient is also one more place we have to look for logic for. I had to open the util file and figure out what exactly the code was doing, etc.

If the concern is that it's a pretty disparate location, I also think there's a danger in that. I really don't think we should treat the server/ folder as something to not look at or stay away from - it's integral for telemetry, for one - and I definitely don't want to be the only dev with familiarity with the code there. In general, I don't think there's any code or area where we should just say "Oh, we don't have to bother looking deeply here".

Also, if we just keep things as is, it makes it easier to just port code over from the old repository. This is one more step for porting views that we could easily cut out.

Definitely a fair argument - for what it's worth I just did a quick grep in ent-search and it looks like these transforms are pretty recent and only occur here (LogRetention) and in the new Crawler folder (and that's something I'd be happy to chat w/ Byron about soon).

With that small scope in mind I think this is worth making this refactor now - it sets up a pattern and precedent that we can use in the future but don't have to. I'm definitely not saying we should go into any of the existing API endpoints or logic files and try to fix all our various inconsistent casing during the migration, but that this allows us to do so after/as a piece of tech debt if we chose to.

I'm also primarily making this proposal now because of the existing convertServerToClient that I think fits perfectly in server/ - if this utility hadn't been here and the existing logic had just dealt with snake_cased json as-is, I wouldn't have proposed this / would have been fine to leave it as-is.

Why not make this change at the source then, in ent-search?

Nevermind, I don't think it's a big concern either way, and it's a cool proposal. Could you please make it a separate PR though, after this is merged?

👍 I'm definitely good with this being separate, and after some thought I'd be fine leaving this off as post-migration tech debt if we'd rather stay focused for now.

Why not make this change at the source then, in ent-search?

That's a good question - the only potential problem I can think of is one we might potentially see as more users move to Kibana: what happens when ent-search and kibana versions are out of sync (e.g. a user upgrades ent-search but not kibana)? Kibana will now start looking for JSON data/keys that don't exist or have changed, and we'll start seeing more bugs/errors as a result.

I'm slightly more inclined to prefer making small changes like casing transforms in the Kibana side as a result (less potential for product mismatch) and keep ent-search more stable.

Great point, thanks 👍

heavy 👍 for leaving this off as post-migration tech debt

...lic/applications/app_search/components/settings/log_retention/utils/convert_log_retention.ts

...arch/public/applications/app_search/components/settings/log_retention/log_retention_logic.ts

...public/applications/app_search/components/settings/log_retention/log_retention_logic.test.ts

cee-chen · 2020-11-04T03:28:15Z

...arch/public/applications/app_search/components/settings/log_retention/log_retention_logic.ts

+  logRetention: ILogRetention | null;
+  logsRetentionUpdating: boolean;
+  openModal: ELogRetentionOptions | null;


[Feel free to disregard this comment totally, just me thinking out loud] This isn't worth the time to change now I think, but if this were new code from scratch I'd probably make the comment the values were a little hard to read/understand the meaning of at a glance.

logsRetentionUpdating would be nice to be update to a bool naming convention, e.g. something like isUpdating (it's fine to be a little generic IMO because the context comes from where we're pulling the value from).

openModal confused me a bit without context because it reads either as an action or possibly a bool. I think it could be clarified to something like currentModalOption maybe?

OK sorry rambling is over now haha

Stuff like this is easy to change now and virtually risk free. It's not changing the code flow in any significant way, it's just renaming things.

9a19871
267d233

Hm, openedModal still reads as a potential bool to me (I know it doesn't have an is/has prefix, but we're not yet consistent w/ that naming in our codebase). I really think something like modalContent or modalOption makes it clearer that the variable being set is a string/piece of copy rather than an open/close flag. But that's definitely not a blocker, so feel free to merge as-is

Yeah I see your point. It's interesting because it needs to signal that the modal is opened, and also which option it is opened for.

Maybe I'll try to switch it in my next PR to modalOption, so I don't have to wait for CI again.

Oh wait. So it is a flag for when the modal is opened? Am I just confused? If so, disregard me totally :dead_inside:

It's like:

when openedModal is null modal is closed is 'api' modal is opened for editing the 'api' option is 'analytics' modal is opened for editing the 'analytics' option

🙈 Ahh gotcha! I would personally find super helpful for dev readability to pull out a selector called isModalOpen that changes to true/false based on modalOption, and use that instead of a null/string var. It's really minor but I think it helps, and hey, vars are cheap!

Co-authored-by: Constance <[email protected]>

...public/applications/app_search/components/settings/log_retention/log_retention_logic.test.ts

This is always complete. When it's returned from the server, it is fully formed, with all options available. We don't need to make the properties optional.

cee-chen

Super appreciate the changes, they look great - the comments I just made are non blocking - feel free to merge at will

cee-chen · 2020-11-05T18:35:56Z

@elasticmachine merge upstream

JasonStoltz · 2020-11-09T13:14:30Z

@elasticmachine merge upstream

kibanamachine · 2020-11-09T14:57:20Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 201bdbe

Metrics [docs]

✅ unchanged

History

💚 Build #86211 succeeded eca9c52
💔 Build #86163 failed 9788589
💚 Build #85804 succeeded be76a85
💚 Build #85603 succeeded 701951d
💚 Build #85579 succeeded bc24dd5e69c88af74f2521b408b0a28fefc812b5

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

JasonStoltz force-pushed the log-retention-logic branch 2 times, most recently from 030fe3c to c10a574 Compare November 2, 2020 20:01

JasonStoltz added release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Nov 3, 2020

JasonStoltz changed the title ~~Log retention logic~~ [Enterprise Search] Log retention settings logic Nov 3, 2020

JasonStoltz force-pushed the log-retention-logic branch from c10a574 to 61efb99 Compare November 3, 2020 16:43

JasonStoltz added 7 commits November 3, 2020 13:38

Added log_retention_logic and setOpenModal

61544bb

Added closeModals to log_retention_logic

df72b99

Note that I changd it to set logsRetentionUpdating to false instead of null so that our types were correct

Added clearLogRetentionUpdating

059a04f

Added updateLogRetention

9024d59

Added saveLogRetention

0ac6bf8

Added toggleLogRetention

fc94421

Added fetchLogRetention

701951d

JasonStoltz force-pushed the log-retention-logic branch from bc24dd5 to 701951d Compare November 3, 2020 18:38

JasonStoltz marked this pull request as ready for review November 3, 2020 18:39

JasonStoltz requested a review from a team November 3, 2020 18:39

cee-chen reviewed Nov 4, 2020

View reviewed changes

JasonStoltz and others added 5 commits November 4, 2020 09:05

Apply suggestions from code review

be76a85

Co-authored-by: Constance <[email protected]>

Dont call toString on enum

6b0989e

Simplified null check

42cc30b

Rename openModal

267d233

Rename logsRetentionUpdating

9a19871

cee-chen reviewed Nov 4, 2020

View reviewed changes

...public/applications/app_search/components/settings/log_retention/log_retention_logic.test.ts Outdated Show resolved Hide resolved

JasonStoltz added 2 commits November 5, 2020 10:47

Moved test to actions block

170d2c2

Update ILogRetention

9788589

This is always complete. When it's returned from the server, it is fully formed, with all options available. We don't need to make the properties optional.

JasonStoltz requested a review from cee-chen November 5, 2020 16:22

cee-chen approved these changes Nov 5, 2020

View reviewed changes

Merge branch 'master' into log-retention-logic

eca9c52

Merge branch 'master' into log-retention-logic

201bdbe

JasonStoltz merged commit d5736b1 into elastic:master Nov 9, 2020

JasonStoltz deleted the log-retention-logic branch November 9, 2020 15:46

JasonStoltz mentioned this pull request Nov 9, 2020

[7.x] [Enterprise Search] Log retention settings logic (#82364) #82983

Merged

JasonStoltz added a commit to JasonStoltz/kibana that referenced this pull request Nov 9, 2020

[Enterprise Search] Log retention settings logic (elastic#82364)

d4f2ad7

JasonStoltz added a commit that referenced this pull request Nov 9, 2020

[Enterprise Search] Log retention settings logic (#82364) (#82983)

e4c37f6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Enterprise Search] Log retention settings logic #82364

[Enterprise Search] Log retention settings logic #82364

JasonStoltz commented Nov 2, 2020 •

edited

Loading

cee-chen left a comment

cee-chen Nov 4, 2020 •

edited

Loading

JasonStoltz Nov 4, 2020

cee-chen Nov 4, 2020 •

edited

Loading

JasonStoltz Nov 4, 2020

cee-chen Nov 4, 2020

JasonStoltz Nov 5, 2020

JasonStoltz Nov 5, 2020 •

edited

Loading

cee-chen Nov 5, 2020 •

edited

Loading

JasonStoltz Nov 5, 2020

richkuz Nov 5, 2020

cee-chen Nov 4, 2020

JasonStoltz Nov 4, 2020

JasonStoltz Nov 5, 2020

cee-chen Nov 5, 2020

JasonStoltz Nov 5, 2020 •

edited

Loading

JasonStoltz Nov 5, 2020

cee-chen Nov 5, 2020

JasonStoltz Nov 5, 2020 •

edited

Loading

cee-chen Nov 5, 2020 •

edited

Loading

cee-chen left a comment

cee-chen commented Nov 5, 2020

JasonStoltz commented Nov 9, 2020

kibanamachine commented Nov 9, 2020

[Enterprise Search] Log retention settings logic #82364

[Enterprise Search] Log retention settings logic #82364

Conversation

JasonStoltz commented Nov 2, 2020 • edited Loading

Summary

Checklist

For maintainers

cee-chen left a comment

Choose a reason for hiding this comment

cee-chen Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Nov 4, 2020 • 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

JasonStoltz Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

cee-chen Nov 5, 2020 • 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

Choose a reason for hiding this comment

JasonStoltz Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonStoltz Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

cee-chen Nov 5, 2020 • edited Loading

Choose a reason for hiding this comment

cee-chen left a comment

Choose a reason for hiding this comment

cee-chen commented Nov 5, 2020

JasonStoltz commented Nov 9, 2020

kibanamachine commented Nov 9, 2020

💚 Build Succeeded

Metrics [docs]

History

JasonStoltz commented Nov 2, 2020 •

edited

Loading

cee-chen Nov 4, 2020 •

edited

Loading

cee-chen Nov 4, 2020 •

edited

Loading

JasonStoltz Nov 5, 2020 •

edited

Loading

cee-chen Nov 5, 2020 •

edited

Loading

JasonStoltz Nov 5, 2020 •

edited

Loading

JasonStoltz Nov 5, 2020 •

edited

Loading

cee-chen Nov 5, 2020 •

edited

Loading