[App Search] API logs: Server route + ApiLogsLogic + useEffects by cee-chen · Pull Request #95732 · elastic/kibana

cee-chen · 2021-03-30T01:21:51Z

Summary

3 main goals of this PR:

Server route: b52ae2c
ApiLogsLogic: 3d77232, 389096d, 2ae83ba
View useEffects: 2250d82

QA

This PR doesn't have any UI changes yet (next set of PRs!) but does add polling behavior that you can QA:

Go to any engine's API logs:
- Confirm an api_logs call fires on load
- Confirm an api_logs call fires every 5 seconds after that
- (TODO in future PR) - Confirm that paginating correctly calls an api_logs call and updates the table
Go to the engine overview:
- Confirm an api_logs call fires on load
- Confirm an api_logs call fires every 5 seconds after that

Checklist

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

cee-chen · 2021-03-30T01:28:53Z

...ugins/enterprise_search/public/applications/app_search/components/api_logs/api_logs_logic.ts

+    pollForApiLogs: () => {
+      if (values.intervalId) return; // Ensure we only have one poll at a time
+
+      const id = window.setInterval(() => actions.fetchApiLogs({ isPoll: true }), POLLING_DURATION);
+      actions.onPollStart(id);
+    },


Changed from the standalone UI:

This now uses setInterval instead of setTimeout to poll, which actually fixes an issue in our current UI where XHRs are getting duplicated (api_logs is called twice every 5s instead of just once 😬)

I separated the poll start into its own action, which I think will be clearer why in a little bit in the actual view/useEffect usage

Yeah I love this.

cee-chen · 2021-03-30T01:33:28Z

...ck/plugins/enterprise_search/public/applications/app_search/components/api_logs/api_logs.tsx

+  useEffect(() => {
+    fetchApiLogs();
+  }, [meta.page.current]);
+
+  useEffect(() => {
+    pollForApiLogs();
+  }, []);


^ I hope with this useEffect example it makes more sense why I split out the concept of an "immediate" fetch and a "poll" fetch in the logic! Also for even more context, check out the different requirements for the Engine Overview:

kibana/x-pack/plugins/enterprise_search/public/applications/app_search/components/engine_overview/components/recent_api_logs.tsx

Lines 24 to 27 in 2250d82

useEffect(() => {

fetchApiLogs();

pollForApiLogs();

}, []);

The table there does not have pagination, so it doesn't need an extra effect and only calls fetchApiLogs once on load.

cee-chen · 2021-03-30T01:38:27Z

...ugins/enterprise_search/public/applications/app_search/components/api_logs/api_logs_logic.ts

+  fetchApiLogs(options?: { isPoll: boolean }): { isPoll: boolean };
+  pollForApiLogs(): void;
+  onPollStart(intervalId: number): { intervalId: number };
+  onPollInterval(data: ApiLogsData): ApiLogsData;
+  storePolledData(data: ApiLogsData): ApiLogsData;
+  updateView(data: ApiLogsData): ApiLogsData;
+  onUserRefresh(): void;
+  onPaginate(newPageIndex: number): { newPageIndex: number };


I played around with the names a bit more from our Slack convo - let me know what you think! I think it is a bit clearer in terms of imperative vs declarative naming (tying user actions/flows to fn names) but definitely open to suggestions, naming things is hard 😬

onPaginationChange would probably have been my pick, this is fine though.

I'm good with w/e - I've been using onPaginate in other places w/ table pagination (overview, curations, etc.) so we should probably change them all at once. Definitely open to a separate follow-up PR for this

cee-chen · 2021-03-30T01:39:31Z

...ugins/enterprise_search/public/applications/app_search/components/api_logs/api_logs_logic.ts

+      if (isEmpty && hasNewData) {
+        actions.updateView(data); // Empty logs should automatically update with new data without a manual action
+      } else if (hasNewData) {
+        actions.storePolledData(data); // Otherwise, store any new data until the user manually refreshes the table


I'm not sure if all these comments are helpful vs. just checking the unit tests - definitely let me know if you have thoughts there. The UX here is significantly different from anything else in our app so I think it could make sense to have that extra context in both places.

I love comments like these

cee-chen · 2021-03-30T01:40:42Z

...ugins/enterprise_search/public/applications/app_search/components/api_logs/api_logs_logic.ts

+      }
+    },
+    onPollInterval: (data, breakpoint) => {
+      breakpoint(); // Prevents errors if logic unmounts while fetching


Found this in https://kea.js.org/docs/guide/additional#breakpoints!!

If you call breakpoint() without any arguments (and without await), there will be no pause. It'll just check if the listener was called again or the logic was unmounted and terminate if that's the case. You can use this version of breakpoint() after long running calls and network requests to avoid those "out of order" errors.

Super useful and we'll definitely want this anywhere we have heartbeat-type polling

cee-chen · 2021-03-30T01:42:21Z

...ck/plugins/enterprise_search/public/applications/app_search/components/api_logs/constants.ts

+  'xpack.enterpriseSearch.appSearch.engine.apiLogs.pollingErrorMessage',
+  {
+    defaultMessage:
+      'Could not automatically refresh API logs data. Please check your connection or manually refresh the page.',


Feel free to give me copy suggestions here, I just kinda yolo'd something

I got nothing here cc @zumwalt

cee-chen · 2021-03-30T01:43:12Z

x-pack/plugins/enterprise_search/public/applications/app_search/components/api_logs/utils.ts

+ * 2.0.
+ */
+
+export const getDateString = (offSetDays?: number) => {


FYI this util comes from the standalone UI, I basically just copied it over as-is, def. open to changes if we want them

cee-chen · 2021-03-30T01:44:08Z

...k/plugins/enterprise_search/public/applications/app_search/components/api_logs/utils.test.ts

+  const mockDate = jest
+    .spyOn(global.Date, 'now')
+    .mockImplementation(() => new Date('1970-01-02').valueOf());


I learned this from James when he used this in a Jest test recently!! 🤯

As someone who loves 1. static tests and 2. Jan 1970, the beginning of the universe, I could NOT pass up using it here

cee-chen · 2021-03-30T01:46:38Z

x-pack/plugins/enterprise_search/public/applications/app_search/components/api_logs/types.ts

+  full_request_path: string;
+  user_agent: string;
+  request_body: string; // JSON string
+  response_body: string; // JSON string


FYI we also get back

path: null,

from our API but I have no idea what it does and we're not using it at all in the standalone UI, so I ignored it. I could have noted it in a comment, but I figured maybe github history (this comment) is sufficient

Worth adding it with a comment IMO

cee-chen · 2021-03-30T01:47:37Z

x-pack/plugins/enterprise_search/public/applications/app_search/components/api_logs/types.ts

+
+export interface ApiLogsData {
+  results: ApiLog[];
+  meta: Meta;


FYI, we get back some extra info in meta:

sort_direction: 'desc', filters: { date: { from: // ... to: // ... }, }, query: '',

But I'm not using it in the frontend (it's data we're sending to the server and not data we need to receive from the server), and I have no idea why query is even there, so I'm ignoring it

byronhulcher

QA looks great! I left some comments which I consider non-blocking

byronhulcher · 2021-03-30T12:59:53Z

...ugins/enterprise_search/public/applications/app_search/components/api_logs/api_logs_logic.ts

+  fetchApiLogs(options?: { isPoll: boolean }): { isPoll: boolean };
+  pollForApiLogs(): void;
+  onPollStart(intervalId: number): { intervalId: number };
+  onPollInterval(data: ApiLogsData): ApiLogsData;
+  storePolledData(data: ApiLogsData): ApiLogsData;
+  updateView(data: ApiLogsData): ApiLogsData;
+  onUserRefresh(): void;
+  onPaginate(newPageIndex: number): { newPageIndex: number };


onPaginationChange would probably have been my pick, this is fine though.

byronhulcher · 2021-03-30T13:01:15Z

...ugins/enterprise_search/public/applications/app_search/components/api_logs/api_logs_logic.ts

+    onPaginate: (newPageIndex) => ({ newPageIndex }),
+  }),
+  reducers: () => ({
+    dataLoading: [


Maybe this is too pedantic but I think if one of the API calls fails, this will be stuck in dataLoading since updateView will never be called. Is that correct and/or does that matter?

byronhulcher · 2021-03-30T13:03:34Z

...ugins/enterprise_search/public/applications/app_search/components/api_logs/api_logs_logic.ts

+    pollForApiLogs: () => {
+      if (values.intervalId) return; // Ensure we only have one poll at a time
+
+      const id = window.setInterval(() => actions.fetchApiLogs({ isPoll: true }), POLLING_DURATION);
+      actions.onPollStart(id);
+    },


Yeah I love this.

byronhulcher · 2021-03-30T13:04:11Z

...ugins/enterprise_search/public/applications/app_search/components/api_logs/api_logs_logic.ts

+      }
+    },
+    onPollInterval: (data, breakpoint) => {
+      breakpoint(); // Prevents errors if logic unmounts while fetching


byronhulcher · 2021-03-30T13:05:02Z

...ugins/enterprise_search/public/applications/app_search/components/api_logs/api_logs_logic.ts

+      if (isEmpty && hasNewData) {
+        actions.updateView(data); // Empty logs should automatically update with new data without a manual action
+      } else if (hasNewData) {
+        actions.storePolledData(data); // Otherwise, store any new data until the user manually refreshes the table


I love comments like these

byronhulcher · 2021-03-30T13:05:45Z

...ck/plugins/enterprise_search/public/applications/app_search/components/api_logs/constants.ts

+  'xpack.enterpriseSearch.appSearch.engine.apiLogs.pollingErrorMessage',
+  {
+    defaultMessage:
+      'Could not automatically refresh API logs data. Please check your connection or manually refresh the page.',


I got nothing here cc @zumwalt

byronhulcher · 2021-03-30T13:06:05Z

x-pack/plugins/enterprise_search/public/applications/app_search/components/api_logs/types.ts

+  full_request_path: string;
+  user_agent: string;
+  request_body: string; // JSON string
+  response_body: string; // JSON string


Worth adding it with a comment IMO

…some day

cee-chen · 2021-03-30T16:30:47Z

Thanks Byron!

Enabling auto-merge as I'm leaving early today, but feel free to shout if y'all think of any naming suggestions or anything else for the future - I can add them into upcoming API logs PRs

byronhulcher · 2021-03-30T16:40:05Z

Enabling auto-merge as I'm leaving early today, but feel free to shout if y'all think of any naming suggestions or anything else for the future - I can add them into upcoming API logs PRs

Sounds great! And thanks for the reminder about auto-merge existing

kibanamachine · 2021-03-30T18:35:51Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`enterpriseSearch`	1291	1293	+2

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.9MB	1.9MB	+3.7KB

History

💚 Build #116004 succeeded 2250d82

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

…tic#95732) * Set up API route * Set up API types * Set up date util needed by filters dates * Add ApiLogsLogic * Update ApiLogs and EngineOverview views with polling behavior * Add API type notes - maybe serves as a TODO to clean up our API data some day

kibanamachine · 2021-03-30T18:38:41Z

💚 Backport successful

✅ 7.x / #95831

This backport PR will be merged automatically after passing CI.

…) (#95831) * Set up API route * Set up API types * Set up date util needed by filters dates * Add ApiLogsLogic * Update ApiLogs and EngineOverview views with polling behavior * Add API type notes - maybe serves as a TODO to clean up our API data some day Co-authored-by: Constance <constancecchen@users.noreply.github.com>

cee-chen added 5 commits March 29, 2021 18:11

Set up API route

b52ae2c

Set up API types

3d77232

Set up date util needed by filters dates

389096d

Add ApiLogsLogic

2ae83ba

Update ApiLogs and EngineOverview views with polling behavior

2250d82

cee-chen added Feature:Plugins release_note:skip Skip the PR/issue when compiling release notes v7.13.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Mar 30, 2021

cee-chen requested a review from a team March 30, 2021 01:21

cee-chen commented Mar 30, 2021

View reviewed changes

byronhulcher self-requested a review March 30, 2021 11:05

byronhulcher approved these changes Mar 30, 2021

View reviewed changes

cee-chen and others added 2 commits March 30, 2021 09:27

Add API type notes - maybe serves as a TODO to clean up our API data …

f87d871

…some day

Merge branch 'master' into api-logs-2

24cd8a5

cee-chen enabled auto-merge (squash) March 30, 2021 16:29

cee-chen merged commit 53d4fa7 into elastic:master Mar 30, 2021

kibanamachine mentioned this pull request Mar 30, 2021

[7.x] [App Search] API logs: Server route + ApiLogsLogic + useEffects (#95732) #95831

Merged

cee-chen deleted the api-logs-2 branch March 30, 2021 18:40

cee-chen mentioned this pull request Jun 23, 2021

[App Search] Refactor empty engine polling to EngineLogic #103041

Merged

2 tasks

Conversation

cee-chen commented Mar 30, 2021

Summary

QA

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cee-chen Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

byronhulcher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cee-chen commented Mar 30, 2021

Uh oh!

byronhulcher commented Mar 30, 2021

Uh oh!

kibanamachine commented Mar 30, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

Uh oh!

kibanamachine commented Mar 30, 2021

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

cee-chen Mar 30, 2021 •

edited

Loading