[APM] Get service name from context for alert flyout #104103
Conversation
dgieselaar
added
release_note:fix
v8.0.0
v7.14.0
auto-backport
|
Pinging @elastic/apm-ui (Team:apm) |
There was a problem hiding this comment.
lgtm!
It seems we have at least 4 ways to access serviceName (and probably other path params):
- Via
useServiceName - As a route prop:
- Via
useParams: - Via
useUrlParams:
Can we make this more consistent? I'm hoping that going forward we can at least get rid of useUrlParams when we tackle #51963
There was a problem hiding this comment.
lgtm!
It seems we have at least 4 ways to access serviceName (and probably other path params):
- Via
useServiceName - As a route prop:
- Via
useParams: - Via
useUrlParams:
Can we make this more consistent going forward? I'm hoping that we can at least get rid of useUrlParams when we tackle #51963
| export function useServiceName(): string | undefined { | ||
| const history = useHistory(); | ||
| for (const config of apmRouteConfig) { | ||
| const match = matchPath<{ serviceName?: string }>( |
There was a problem hiding this comment.
Would it be better to use useRouteMatch? I don't think we would need to iterate through the routes if we did that. It's independent of any route component and just uses the current path.
There was a problem hiding this comment.
useRouteMatch() returns an empty match object:
There was a problem hiding this comment.
Don't you have to give it a path like useRouteMatch('/app/apm/services/:serviceName/*')? I thought that's how it worked.
There was a problem hiding this comment.
I think in this case we only know the actual pathname (history.location.pathname), not the route since it's a generic hook that can be called anywhere in the application.
There was a problem hiding this comment.
... but seeing your example, if it allows wildcards it might be different.
There was a problem hiding this comment.
nvm. It seems like useRouteMatch simply uses https://github.com/pillarjs/path-to-regexp under the hood. What @smith is suggesting makes sense
There was a problem hiding this comment.
I've changed it to useRouteMatch('/services/:serviceName'), which seems to work fine.
Yeah, I'd prefer to do it there and do the minimal changes now, as we're past FF. |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
Closes #101837.
Instead of using
useParamsto get the service name, introduce auseServiceNamehook that is used byuseApmServiceContext, in whichmatchPathandapmRouteConfigare used. This removes the necessity of the component callinguseApmServiceContextto be a child of a service view route.