Skip to content
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

Open a new APM transaction for every meaningful page instead of every currentAppId$ #114843

Closed
lizozom opened this issue Oct 13, 2021 · 6 comments
Labels
discuss enhancement New value added to drive a business result impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort performance Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0

Comments

@lizozom
Copy link
Contributor

lizozom commented Oct 13, 2021

Currently route-change transactions are started everytime the currentAppId$ changes and are closed upon largestContentPaint. This diagram illustrates it - You can see that navigations within the app don't create an APM transaction at all.

image

We want to change the integration in such a way that a new transaction is started every time a page changes. This would require a way of letting apps communicate page changes. @Bamieh mentioned the TrackApplicationView component, but that one may open multiple view contexts one withing the other (for example a flyout within a page can use a TrackApplicationView).

The end result should look something like this (includes changing page-load behavior as described here:

image

Once we have this, we will proceed to track individual events within each transaction.

@lizozom lizozom added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0 reason:enhancement labels Oct 13, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@pgayvallet
Copy link
Contributor

Atm the route-change transaction is handled outside of the application service, directly in apm_system

start.application.currentAppId$.subscribe((appId) => {
const apmInstance = (window as any).elasticApm;
if (appId && apmInstance && typeof apmInstance.startTransaction === 'function') {
apmInstance.startTransaction(`/app/${appId}`, 'route-change', {
managed: true,
canReuse: true,
});
}
});

Having a better granularity would require to move that to the application service to have better control over it.

Now,

Technically:

we need to hook into something. We could eventually hook into navigateToApp, but most intra-app navigations are NOT using this, and are directly using their nested router, meaning that we would miss all of those, so this is not an option.

we could listen to the root history itself, but this would be extremely verbose. e.g a parameter change would cause a new event to fire. Plus, if we hook into the history, we would be very low level and would not have the info about the current app. We would just have the path and state of the current location.

Plugging into the mounted app wrapper would not work either, as an application if not re-mounted when their internal path changes. And we don't have the path information here (even if we could probably find a way to access it somehow).

const wrapMount = (plugin: PluginOpaqueId, app: App<any>): AppMount => {
return async (params) => {
this.currentAppId$.next(app.id);
return app.mount(params);
};
};

Functionally

I'm honestly not sure to see why core should manage internal application transactions. I think applications may want different level of granularity in their transactions, and I doubt we could provide something that would suit everyone?

Now, I agree that route-change is misleading. But I'm not sure we could (or should) do anything else at core's level than rename it to app-change?

Curious to see what the other team members think about this.

@lizozom
Copy link
Contributor Author

lizozom commented Oct 14, 2021

I strongly agree @pgayvallet - I don't think this core should manage internal application transactions..

Instead, for example, we could have some observable (it can be part of the reporting package we want to create) to allow apps easily emit page changes on it.

There are multiple technical solutions, but eventually, in the context of event reporting (be it to APM, fullstory or telemetry-next), we still need a reliable way to set the start point of a well-defined page visit.

@exalate-issue-sync exalate-issue-sync bot added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort labels Nov 4, 2021
@lizozom
Copy link
Contributor Author

lizozom commented Feb 8, 2022

Partially addressed by #124996

@afharo
Copy link
Member

afharo commented Feb 10, 2022

AFAIK, in Core we use react-router-dom's Router - Switch - Route to route to different apps:

return (
<Router history={history}>
<Switch>
{[...mounters].map(([appId, mounter]) => (
<Route
key={mounter.appRoute}
path={mounter.appRoute}
exact={mounter.exactRoute}
render={({ match: { path } }) => (
<AppContainer
appPath={path}
appStatus={appStatuses.get(appId) ?? AppStatus.inaccessible}
createScopedHistory={createScopedHistory}
{...{ appId, mounter, setAppLeaveHandler, setAppActionMenu, setIsMounting, theme$ }}
/>
)}
/>
))}
{/* handler for legacy apps and used as a catch-all to display 404 page on not existing /app/appId apps*/}
<Route
path="/app/:appId"
render={({
match: {
params: { appId },
url,
},
}: RouteComponentProps<Params>) => {
// the id/mounter retrieval can be removed once #76348 is addressed
const [id, mounter] = mounters.has(appId) ? [appId, mounters.get(appId)] : [];
return (
<AppContainer
appPath={url}
appId={id ?? appId}
appStatus={appStatuses.get(appId) ?? AppStatus.inaccessible}
createScopedHistory={createScopedHistory}
{...{ mounter, setAppLeaveHandler, setAppActionMenu, setIsMounting, theme$ }}
/>
);
}}
/>
</Switch>
</Router>

I wonder if we could use the ApmRoute component exposed by @elastic/apm-rum-react as a direct replacement of Route to get some "Application Changed" metrics "for free". What do you think?

@pgayvallet
Copy link
Contributor

@lizozom WDYT? Do you know if using ApmRoute would provide more value that what we're manually doing in src/core/public/apm_system.ts ?

@lizozom lizozom closed this as completed Jun 23, 2022
@sophiec20 sophiec20 added the enhancement New value added to drive a business result label Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss enhancement New value added to drive a business result impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort performance Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.0.0
Projects
None yet
Development

No branches or pull requests

5 participants