Skip to content

[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.

Sign up for GitHub

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

Merged
ogupte merged 1 commit into from
Jan 12, 2021
Merged

[APM] Fix stale custom links list after creating new link #87932

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...lugins/apm/public/components/shared/TransactionActionMenu/CustomLinkMenuSection/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export function CustomLinkMenuSection({
const { data: customLinks = [], status, refetch } = useFetcher(
(callApmApi) =>
callApmApi({
isCachable: true,
isCachable: false,
Copy link
Member

@sorenlouv sorenlouv Jan 12, 2021

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.

Copy link
Contributor

@cauemarcondes cauemarcondes Jan 12, 2021

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?

Copy link
Member

@sorenlouv sorenlouv Jan 12, 2021

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 👍

Copy link
Contributor Author

@ogupte ogupte Jan 12, 2021

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.

Copy link
Member

@sorenlouv sorenlouv Jan 12, 2021

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?

Copy link
Member

@sorenlouv sorenlouv Jan 12, 2021 •
edited
Loading

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...

endpoint: 'GET /api/apm/settings/custom_links',
params: { query: convertFiltersToQuery(filters) },
}),
Expand Down