-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[APM] Fix stale custom links list after creating new link #87932
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
[APM] Fix stale custom links list after creating new link #87932
Conversation
|
Pinging @elastic/apm-ui (Team:apm) |
cauemarcondes
left a comment
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.
LGTM
| (callApmApi) => | ||
| callApmApi({ | ||
| isCachable: true, | ||
| isCachable: false, |
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.
@cauemarcondes Didn't you recently add this? I suppose we added it for a reason - maybe we were seeing duplicate requests?
I'm wondering if we can keep the caching and solve it by improved cache invalidation. Afair calling refetch should override the existing cache - if not that's probably a bug.
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.
@cauemarcondes Didn't you recently add this?
@sqren No, actually you added it when you refactored the custom link components. But I don't think removing it would be a problem, we already lazy-loading the API call (it's only called when the Action menu is opened). WDYT?
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, alzheimer's coming early 😴
Okay, removing it sgtm 👍
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.
refetch is working as expected, the issue here is with the callApmApi result being cached. It is invoked each time refetch is called, but with isCachable: true it returns immediately. There are 2 levels of caching here, calling refetch invalidates the first level, and setting isCachable: false disables the 2nd level.
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.
Outside the scope of this PR but shouldn't refetch always circumvent the cache?
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.
... thinking about this some more, it might be hard to handle automatically. We could pass a boolean to the handler that indicated whether the data we refetched:
const { data = DEFAULT_DATA, status } = useFetcher(
(callApmApi, refetched) =>
callApmApi({
isCachable: !refetched,
endpoint: `GET /api/apm/foo`
}),
[]
);But not sure how i feel about that...
sorenlouv
left a comment
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.
lgtm but please double check that refetch works as it should
Closes #86989. Disable caching for fetched custom links