-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[App Search] API logs: Server route + ApiLogsLogic + useEffects #95732
Conversation
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); | ||
}, |
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.
Changed from the standalone UI:
- This now uses
setInterval
instead ofsetTimeout
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
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.
Yeah I love this.
useEffect(() => { | ||
fetchApiLogs(); | ||
}, [meta.page.current]); | ||
|
||
useEffect(() => { | ||
pollForApiLogs(); | ||
}, []); |
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.
^ 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:
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.
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 }; |
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.
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 😬
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.
onPaginationChange
would probably have been my pick, this is fine though.
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.
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
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 |
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.
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.
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.
I love comments like these
} | ||
}, | ||
onPollInterval: (data, breakpoint) => { | ||
breakpoint(); // Prevents errors if logic unmounts while fetching |
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.
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
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.
GREAT find
'xpack.enterpriseSearch.appSearch.engine.apiLogs.pollingErrorMessage', | ||
{ | ||
defaultMessage: | ||
'Could not automatically refresh API logs data. Please check your connection or manually refresh the page.', |
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.
Feel free to give me copy suggestions here, I just kinda yolo'd something
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.
I got nothing here cc @zumwalt
* 2.0. | ||
*/ | ||
|
||
export const getDateString = (offSetDays?: number) => { |
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.
FYI this util comes from the standalone UI, I basically just copied it over as-is, def. open to changes if we want them
const mockDate = jest | ||
.spyOn(global.Date, 'now') | ||
.mockImplementation(() => new Date('1970-01-02').valueOf()); |
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.
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
full_request_path: string; | ||
user_agent: string; | ||
request_body: string; // JSON string | ||
response_body: string; // JSON string |
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.
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
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.
Worth adding it with a comment IMO
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.
|
||
export interface ApiLogsData { | ||
results: ApiLog[]; | ||
meta: Meta; |
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.
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
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.
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 }; |
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.
onPaginationChange
would probably have been my pick, this is fine though.
onPaginate: (newPageIndex) => ({ newPageIndex }), | ||
}), | ||
reducers: () => ({ | ||
dataLoading: [ |
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.
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?
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); | ||
}, |
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.
Yeah I love this.
} | ||
}, | ||
onPollInterval: (data, breakpoint) => { | ||
breakpoint(); // Prevents errors if logic unmounts while fetching |
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.
GREAT find
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 |
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.
I love comments like these
'xpack.enterpriseSearch.appSearch.engine.apiLogs.pollingErrorMessage', | ||
{ | ||
defaultMessage: | ||
'Could not automatically refresh API logs data. Please check your connection or manually refresh the page.', |
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.
I got nothing here cc @zumwalt
full_request_path: string; | ||
user_agent: string; | ||
request_body: string; // JSON string | ||
response_body: string; // JSON string |
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.
Worth adding it with a comment IMO
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 |
Sounds great! And thanks for the reminder about auto-merge existing |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
…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
…) (#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>
Summary
3 main goals of this PR:
QA
This PR doesn't have any UI changes yet (next set of PRs!) but does add polling behavior that you can QA:
api_logs
call fires on loadapi_logs
call fires every 5 seconds after thatapi_logs
call and updates the tableapi_logs
call fires on loadapi_logs
call fires every 5 seconds after thatChecklist