[App Search] Engine Overview server route & Logic file #83353

cee-chen · 2020-11-12T22:19:43Z

Summary

Setup PR for the Engine Overview view. It adds a server route to fetch overview metrics data and a logic file as setup for the Engine Overview page.

There's nothing in the UI yet to test.

Checklist

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

cee-chen · 2020-11-12T22:28:09Z

...se_search/public/applications/app_search/components/engine_overview/engine_overview_logic.ts

+  totalClicks: number;
+  totalQueries: number;
+}
+interface EngineOverviewValues extends EngineOverviewApiData {


FYI - this drops/cleans up a good amount of values from ent-search:

apiKey & apiUrl - this wasn't being used anywhere, not actually sure where it was coming from (maybe leftover from old logic). apiKey should just be grabbed directly from EngineLogic.values.engine.apiKey in any case.

accountKey - no longer applies in a SMAS world

role - also not used anywhere AFAICT, should just be pulled directly from AppLogic.values.myRole

pollingCancelTokenSource - per a discussion w/ Byron, we will no longer be using/storing axios cancel tokens

showDocumentCreationModal & creationMode - I'm going to be spiking out something in a few PRs here where DocumentCreationModal handles its own show/hide state and creation mode state instead of parent views doing it in multiple places.

activeApiLog - Same as the above, I believe ApiLogTables should be in charge of managing the state of its details modal, not the parent. I'm removing it for now on that premise, but if for any reason my belief is incorrect we can come back in later and add it

cee-chen · 2020-11-12T22:30:27Z

...se_search/public/applications/app_search/components/engine_overview/engine_overview_logic.ts

+import { HttpLogic } from '../../../shared/http';
+import { EngineLogic } from '../engine';
+
+const POLLING_DURATION = 5000;


Per a convo in the front-end channel, I think we're just gonna set per-view POLLING_DURATION timing instead of trying to share a single const. We can come back later and look at this if this changes for any reason

cee-chen · 2020-11-12T22:34:47Z

...arch/public/applications/app_search/components/engine_overview/engine_overview_logic.test.ts

+jest.mock('../../../shared/http', () => ({
+  HttpLogic: { values: mockHttpValues },
+}));
+const { http } = mockHttpValues;
+
+jest.mock('../../../shared/flash_messages', () => ({
+  flashAPIErrors: jest.fn(),
+}));
+import { flashAPIErrors } from '../../../shared/flash_messages';
+
+jest.mock('../engine', () => ({


I would really like to figure out someday how to DRY out all these mocks to a reusable helper, especially for the shared ones, but I'm not sure we'll ever be able to because of the relative ../ paths.

Maybe someday we can ask the Kibana team if absolute webpack alias's are possible for xpack plugins - we can but dream!

cee-chen · 2020-11-13T01:44:26Z

...se_search/public/applications/app_search/components/engine_overview/engine_overview_logic.ts

+  }),
+  events: ({ values }) => ({
+    beforeUnmount() {
+      if (values.timeoutId !== null) clearTimeout(values.timeoutId);


I saw a post about nullish operators/assignment on Twitter recently and almost kinda wanted to try it out here:

Suggested change

if (values.timeoutId !== null) clearTimeout(values.timeoutId);

values.timeoutId ?? clearTimeout(values.timeoutId);

But it felt kinda weird to read and Typescript didn't like it. haha

I wonder why TS didn't like it?

One interesting thing...

In the former, if values.timeoutId is undefined, clearTimeout would be called.

In the later, if values.timeoutId is undefined, clearTimeout would NOT be called.

Looking at the code now, I realize that is irrelevant, because timeoutId would never be null

TS probably isn't yet fully updated for nullish operators is my guess.

timeoutId would never be null

I think you meant undefined here? If so, correct, a redux value can never be undefined or the entire app just crashes

There's often a large disconnect between my brain and my fingers 😬

JasonStoltz · 2020-11-16T16:40:52Z

x-pack/plugins/enterprise_search/server/routes/app_search/engines.ts

@@ -60,4 +60,19 @@ export function registerEnginesRoutes({
      })(context, request, response);
    }
  );
+  router.get(
+    {
+      path: '/api/app_search/engines/{name}/overview',


I mentioned this before, I'll mention it again to see what you think.

I don't particularly like renaming routes. If this forwards to /overview_metrics in Enterprise Search, I think it should also be /overview_metrics in the Kibana API.

It just simplifies things, IMO. One less thing to think about. If you see /overview_metrics throwing an error, it's simpler to just go look for the /overview_metrics API endpoint in the backend to debug, rather than first checking the server mapping to see what the real API endpoint is on the backend to debug.

That's my totally subjective opinion. Feel free to disregard and keep it as is. I don't think it ultimately will make a big difference one way or the other.

That's a super fair point - what I was thinking originally when looking at this endpoint / view was:

What guarantees that our Overview page will only ever show metrics in the future? Why not show/pass other non-metric information?

If we ever do at that point, we would (likely?) change the name of the endpoint, and a generic /overview (to match the page) is more future-proof

That being said, I'm hoping we take a second pass at all our API endpoints and where possible convert internal ones to standard API endpoints in some distant future - at that point it would be great to look at names as well.

JasonStoltz · 2020-11-16T16:46:02Z

...se_search/public/applications/app_search/components/engine_overview/engine_overview_logic.ts

+    },
+    onPollingSuccess: (engineMetrics) => {
+      const timeoutId = window.setTimeout(actions.pollForOverviewMetrics, POLLING_DURATION);
+      actions.setTimeoutId(timeoutId);


Don't change this, this is non-blocking feedback. Just wanted to point out a best practice from Kea and Redux.

https://kea.js.org/docs/guide/concepts#listeners

If you read the reducers section above, you'll remember that it's an anti-pattern to only have setThis and setThat actions that only update this or that.

Often times when you need to call 2 or more actions like this on success to set resulting data, it's a good indication that you're following this anti-pattern.

There really is only 1 "action" happening here. Meaning, if you think of actions as "events" like Kea suggestion, the "event" that is happening here is that polling has completed. Reducers should react to that "event" and set data accordingly.

I could be mis-reading this, just thought I'd take a moment to point this out.

Ahh super interesting! I went back and forth a bit on whether I should refactor/clean this up as well actually but ended up leaving it as-is because it didn't bother me too much.

If you read the reducers section above, you'll remember that it's an anti-pattern to only have setThis and setThat actions that only update this or that.

This confuses me a bit, we do this in a number of our Kea files and I'm not quite clear on what the "correct" code looks like (maybe just a bit too early in the morning for me).

I did just discover sharedListeners in their docs which blew my mind (how long has that been there!?) and looks like we could use it in our code... I'd love to do an overall Kea refactor at some point if we have time after all this is over 😅

JasonStoltz · 2020-11-16T17:00:19Z

...se_search/public/applications/app_search/components/engine_overview/engine_overview_logic.ts

+export const EngineOverviewLogic = kea<MakeLogicType<EngineOverviewValues, EngineOverviewActions>>({
+  path: ['enterprise_search', 'app_search', 'engine_overview_logic'],
+  actions: () => ({
+    setPolledData: (engineMetrics) => engineMetrics,


Non-blocking feedback. But it usually only takes a moment to do, so when I convert these logic files I often change actions to return objects based on Kea's recommendation:

One more thing. It's strongly recommended to always return an object as a payload from your actions:

Suggested change

setPolledData: (engineMetrics) => engineMetrics,

setPolledData: (engineMetrics) => { engineMetrics },

Oh this is super bizarre - I thought I left a comment on this but it appears to have disappeared :| So unfortunately based on the way the logic file is written we actually can't do this here without significantly rewriting / adding more destructuring for every single top-level value in the file. I haven't seen any other logic file do this, but in this one, we pull almost every top-level key out of the API response and set it as its own value (see the test).

Which means if we passed engineMetrics as an obj key, we now have to destructure that and do something like:

setPolledData: (_, { engineMetrics: { apiLogsUnavailable } }) => apiLogsUnavailable,

Which isn't the worst, but I went back and forth on this and ended up just deciding to leave it as it was from the current code. I definitely think this page and logic file could stand to be looked at first from a refactor POV in the future, as I believe it's one of our earliest/oldest views.

Makes sense!

JasonStoltz

I dropped some non-blocking feedback. Please feel free to merge as-is. This all looks great.

cee-chen · 2020-11-16T17:26:23Z

Thanks Jason! I've created some API endpoint & Kea tech debt tickets with links and details to our conversations above to investigate post 7.14 / 8.x. I'm hoping to look at those items then both for this file specifically & in general across our codebase.

Feel free to use those tickets too if you encounter any files to look at later as part of our migration!

cee-chen · 2020-11-16T17:27:39Z

@elasticmachine merge upstream

kibanamachine · 2020-11-16T19:11:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 4ea4ab3

Metrics [docs]

✅ unchanged

History

💚 Build #87789 succeeded 52f30e0
💔 Build #87771 failed b774722

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

* Add overview server route * Add EngineOverviewLogic * tfw when you forget index.ts Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

Add overview server route

0d5c1d2

cee-chen added release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Nov 12, 2020

cee-chen requested review from JasonStoltz and a team November 12, 2020 22:19

Add EngineOverviewLogic

b774722

cee-chen force-pushed the engine-overview branch from 8d53954 to b774722 Compare November 12, 2020 22:31

cee-chen commented Nov 12, 2020

View reviewed changes

tfw when you forget index.ts

52f30e0

cee-chen commented Nov 13, 2020

View reviewed changes

JasonStoltz reviewed Nov 16, 2020

View reviewed changes

JasonStoltz approved these changes Nov 16, 2020

View reviewed changes

Merge branch 'master' into engine-overview

4ea4ab3

cee-chen merged commit bc3bb2a into elastic:master Nov 16, 2020

cee-chen deleted the engine-overview branch November 16, 2020 19:33

cee-chen mentioned this pull request Nov 16, 2020

[7.x] [App Search] Engine Overview server route & Logic file (#83353) #83471

Merged

JasonStoltz mentioned this pull request Mar 24, 2021

[App Search] Add a Sample Engine CTA panel to the engines table when empty #94647

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[App Search] Engine Overview server route & Logic file #83353

[App Search] Engine Overview server route & Logic file #83353

cee-chen commented Nov 12, 2020

cee-chen Nov 12, 2020

cee-chen Nov 12, 2020

cee-chen Nov 12, 2020

cee-chen Nov 13, 2020

JasonStoltz Nov 16, 2020

JasonStoltz Nov 16, 2020

cee-chen Nov 16, 2020

JasonStoltz Nov 16, 2020

JasonStoltz Nov 16, 2020

cee-chen Nov 16, 2020

JasonStoltz Nov 16, 2020 •

edited

Loading

cee-chen Nov 16, 2020 •

edited

Loading

JasonStoltz Nov 16, 2020

cee-chen Nov 16, 2020 •

edited

Loading

JasonStoltz Nov 16, 2020

JasonStoltz left a comment

cee-chen commented Nov 16, 2020 •

edited

Loading

cee-chen commented Nov 16, 2020

kibanamachine commented Nov 16, 2020

	if (values.timeoutId !== null) clearTimeout(values.timeoutId);
	values.timeoutId ?? clearTimeout(values.timeoutId);

	setPolledData: (engineMetrics) => engineMetrics,
	setPolledData: (engineMetrics) => { engineMetrics },

[App Search] Engine Overview server route & Logic file #83353

[App Search] Engine Overview server route & Logic file #83353

Conversation

cee-chen commented Nov 12, 2020

Summary

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

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 16, 2020 • edited Loading

Choose a reason for hiding this comment

cee-chen Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cee-chen Nov 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JasonStoltz left a comment

Choose a reason for hiding this comment

cee-chen commented Nov 16, 2020 • edited Loading

cee-chen commented Nov 16, 2020

kibanamachine commented Nov 16, 2020

💚 Build Succeeded

Metrics [docs]

History

JasonStoltz Nov 16, 2020 •

edited

Loading

cee-chen Nov 16, 2020 •

edited

Loading

cee-chen Nov 16, 2020 •

edited

Loading

cee-chen commented Nov 16, 2020 •

edited

Loading