[APM] Abort browser requests when appropriate by dgieselaar · Pull Request #89557 · elastic/kibana

dgieselaar · 2021-01-28T13:39:57Z

elasticmachine · 2021-01-28T13:40:01Z

Pinging @elastic/uptime (Team:uptime)

elasticmachine · 2021-01-28T13:40:02Z

Pinging @elastic/apm-ui (Team:apm)

justinkambic

I'm rubber stamping this because it triggered a CODEOWNER review on one file. I'm operating under the assumption if signal: null poses no threat to the rest of the routes changed it will operate similarly in the UX API.

sorenlouv · 2021-01-29T09:05:35Z

x-pack/plugins/apm/public/hooks/use_fetcher.tsx

  );
 }

+const createLifecycleManagedCallApmApi = (


nit (suggestion)

Suggested change

const createLifecycleManagedCallApmApi = (

const createAbortableApmApiClient = (

I considered this, but every ApmApiClient's calls are abortable, if you pass a signal. This one takes care of aborting for you. So I'm not sure if createAbortableApmApiClient is better here. But I agree that createLifecycleManagedCallApmApi is not great.

Ah, okay. Would it make sense to call it createAutoAbortableApmClient or something like that? "lifecycle" is pretty very vague, and not something I'd immediately associate with the request being aborted automatically.

Agreed, let me think about it, I think that's one better at least.

sorenlouv · 2021-01-29T09:05:52Z

x-pack/plugins/apm/public/hooks/use_fetcher.tsx


+const createLifecycleManagedCallApmApi = (
+  signal: AbortSignal
+): LifecycleManagedAPMClient => {


Suggested change

): LifecycleManagedAPMClient => {

): AbortableAPMClient => {

sorenlouv · 2021-01-29T09:06:22Z

x-pack/plugins/apm/public/hooks/use_fetcher.tsx


 export function useFetcher<TReturn>(
-  fn: (callApmApi: APMClient) => TReturn,
+  fn: (callApmApi: LifecycleManagedAPMClient) => TReturn,


Suggested change

fn: (callApmApi: LifecycleManagedAPMClient) => TReturn,

fn: (apmApiClient: AbortableAPMClient) => TReturn,

sorenlouv · 2021-01-29T09:10:23Z

x-pack/plugins/apm/public/hooks/use_fetcher.tsx


    return () => {
-      didCancel = true;
+      controller.abort();


What will the FETCH_STATUS state be when a request is aborted? Should we introduce a new state or is that overkill (I'd rather not if we can avoid it).

Aborted is not necessarily a state here, it can be a result of a new request, the component unmounting, or the useFetcher cb not returning a promise. We were already "soft-canceling" requests before, so I don't think this change should affect state but I'll think about it for a bit.

Okay. I was trying to think through if we still needed the if (!signal.aborted) { checks. Previously, if useFetcher props changed, we used didCancel = true to avoid updating the state when the underlying request (which didn't get cancelled) returned.
Since we are now actually cancelling the request, will useFetcher still re-render upon cancellation (or some later stage) or can we omit the checks?

We might not need the first one. But the second one we do, as the promise will be rejected when the request is aborted.

sorenlouv · 2021-01-29T09:15:41Z

x-pack/plugins/apm/public/services/rest/createCallApmApi.ts

 export type APMClient = Client<APMAPI['_S']>;
+export type LifecycleManagedAPMClient = Client<
+  APMAPI['_S'],
+  { lifecycleManaged: true }


Suggested change

{ lifecycleManaged: true }

{ isAbortable: true }

sorenlouv

LGTM!

When I started in Elastic in 2017 we couldn't use this feature because it wasn't well supported. So nice to see how the web is still moving forward - I sometimes forget that :D

…r-request

sorenlouv

lgtm. Just on question around how cancellation affects re-rendering.

dgieselaar · 2021-02-01T09:23:47Z

@elasticmachine merge upstream

…r-request

… into cancel-browser-request

kibanamachine · 2021-02-01T18:13:57Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3036a03

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`apm`	1739	1738	-1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`apm`	5.2MB	5.2MB	-602.0B

History

💚 Build #102507 succeeded 97de3b5
💔 Build #102101 failed d291930
💚 Build #101744 succeeded 13195fe

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

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

…-ml-jobs * 'master' of github.com:elastic/kibana: (254 commits) [Security Solution] [Detections] Remove allow_no_indices to prevent error being thrown in response of field capabilities (elastic#89927) Skip test for cloud (elastic#89450) [Fleet] Fix duplicate data streams being shown in UI (elastic#89812) Bump package dependencies (elastic#90034) [App Search] DRY helper for encoding/decoding routes that can have special characters in params (elastic#89811) TypeScript project references for Observability plugin (elastic#89320) [SearchSource] Combine sort and parent fields when serializing (elastic#89808) Made imports static (elastic#89935) [ml] migrate file_data_visualizer/import route to file_upload plugin (elastic#89640) [Discover] Adapt default column behavior (elastic#89826) Round start and end values (elastic#89030) Rename getProxyAgents to getCustomAgents (elastic#89813) [Form lib] UseField `onError` listener (elastic#89895) [APM] use latency sum instead of avg for impact (elastic#89990) migrate more core-owned plugins to tsproject ref (elastic#89975) [Logs UI] Load <LogStream> entries via async searches (elastic#86899) [APM] Abort browser requests when appropriate (elastic#89557) [Alerting] Allow user to select existing connector of same type when fixing broken connector (elastic#89062) [Data Table] Use shared CSV export mechanism (elastic#89702) chore(NA): improve logic check when installing Bazel tools (elastic#89634) ...

[APM] Abort browser requests when appropriate

13195fe

Close elastic#81740.

dgieselaar added release_note:skip Skip the PR/issue when compiling release notes v7.12.0 labels Jan 28, 2021

dgieselaar requested a review from a team January 28, 2021 13:39

dgieselaar requested a review from a team as a code owner January 28, 2021 13:39

botelastic bot added Team:APM - DEPRECATED Use Team:obs-ux-infra_services. Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability labels Jan 28, 2021

justinkambic approved these changes Jan 28, 2021

View reviewed changes

sorenlouv reviewed Jan 29, 2021

View reviewed changes

sorenlouv approved these changes Jan 29, 2021

View reviewed changes

dgieselaar added 2 commits January 29, 2021 12:46

Review feedback

6b981d9

Merge branch 'master' of github.com:elastic/kibana into cancel-browse…

d291930

…r-request

sorenlouv approved these changes Jan 29, 2021

View reviewed changes

kibanamachine and others added 4 commits February 1, 2021 04:23

Merge branch 'master' into cancel-browser-request

97de3b5

Merge branch 'master' of github.com:elastic/kibana into cancel-browse…

b3c70e0

…r-request

Clarify check for signal.aborted, make client abort test more resilient

d1d7e4b

Merge branch 'cancel-browser-request' of github.com:dgieselaar/kibana…

3036a03

… into cancel-browser-request

dgieselaar merged commit 2a4d39a into elastic:master Feb 2, 2021

dgieselaar deleted the cancel-browser-request branch February 2, 2021 13:44

dgieselaar mentioned this pull request Feb 2, 2021

[7.x] [APM] Abort browser requests when appropriate (#89557) #90011

Merged

dgieselaar added a commit to dgieselaar/kibana that referenced this pull request Feb 2, 2021

[APM] Abort browser requests when appropriate (elastic#89557)

1e0572d

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

	const createLifecycleManagedCallApmApi = (
	const createAbortableApmApiClient = (

	fn: (callApmApi: LifecycleManagedAPMClient) => TReturn,
	fn: (apmApiClient: AbortableAPMClient) => TReturn,

Conversation

dgieselaar commented Jan 28, 2021

Uh oh!

elasticmachine commented Jan 28, 2021

Uh oh!

elasticmachine commented Jan 28, 2021

Uh oh!

justinkambic 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!

Choose a reason for hiding this comment

Uh oh!

sorenlouv Jan 29, 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!

sorenlouv Jan 29, 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!

sorenlouv left a comment

Choose a reason for hiding this comment

Uh oh!

sorenlouv left a comment

Choose a reason for hiding this comment

Uh oh!

dgieselaar commented Feb 1, 2021

Uh oh!

kibanamachine commented Feb 1, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sorenlouv Jan 29, 2021 •

edited

Loading

sorenlouv Jan 29, 2021 •

edited

Loading