[APM] Setting for default env for service inventory #126151
Conversation
dgieselaar
added
release_note:enhancement
Team:APM
|
Pinging @elastic/apm-ui (Team:apm) |
|
|
||
| if (last(matchingRoutes)?.path === '/services') { | ||
| const defaultServiceEnvironment = | ||
| core.uiSettings.get<boolean>(defaultApmServiceEnvironment) || |
There was a problem hiding this comment.
Yep, thanks! Good catch. Was copying pasta :)
| @@ -27,6 +27,7 @@ export { | |||
| enableInspectEsQueries, | |||
| enableComparisonByDefault, | |||
| enableInfrastructureView, | |||
| defaultApmServiceEnvironment, | |||
There was a problem hiding this comment.
It's not related only to this change but since these keys are being exported from x-pack/plugins/observability/common/index.ts are there benefits to have them here as well?
There was a problem hiding this comment.
I'm not sure I follow - mind elaborating?
There was a problem hiding this comment.
I was just wondering if we do need these exports as it seems that the ones from /common can be used instead.
|
|
||
| if (last(matchingRoutes)?.path === '/services') { | ||
| const defaultServiceEnvironment = | ||
| core.uiSettings.get<boolean>(defaultApmServiceEnvironment) || |
There was a problem hiding this comment.
Shouldn't this be a string?
This is a good point - however it is already the case w/ the environment selector in general. It probably doesn't make sense for me to address it in this PR given the planned changes to the environment selector, so I'll go ahead and merge this in as-is. |
|
|
||
| const matchingRoutes = apmRouter.getRoutesToMatch(location.pathname); | ||
|
|
||
| if (last(matchingRoutes)?.path === '/services') { |
There was a problem hiding this comment.
Why only check if the last matches?
There was a problem hiding this comment.
Would be nice if we had a helper like this:
const isServiceInventory = apmRouter.isRoute(location.pathname, '/services')There was a problem hiding this comment.
In this case, it's to ensure it only matches when /services is being rendered, and not one of its children. /services doesn't have children now, but when they are added, the redirect might be too aggressive. Maybe it's a little too defensive though, I'll see if there's a better way. I think a helper that takes a route path doesn't really help here, there could be multiple routes that have the same route path (e.g. /).
| const location = useLocation(); | ||
| const history = useHistory(); | ||
|
|
||
| const query = qs.parse(location.search); |
There was a problem hiding this comment.
What about this?
| const query = qs.parse(location.search); | |
| const { query } = useApmParams(`*`); |
There was a problem hiding this comment.
Replied via email, will post here as well:
These components are rendered before a route is actually matched, providing
the opportunity to catch invalid route params (eg a missing query param)
and then redirecting to a valid state. The downside is that useApmParams
cannot (or should not) be used.
| }), | ||
| description: i18n.translate('xpack.observability.defaultApmServiceEnvironmentDescription', { | ||
| defaultMessage: | ||
| 'Set the default environment for the APM app. When left empty, data from all environments will be displayed by default.', |
|
These components are rendered before a route is actually matched, providing
the opportunity to catch invalid route params (eg a missing query param)
and then redirecting to a valid state. The downside is that useApmParams
cannot (or should not) be used.
Op vr 25 feb. 2022 23:11 schreef Søren Louv-Jansen ***@***.***
…:
***@***.**** commented on this pull request.
------------------------------
In
x-pack/plugins/apm/public/components/shared/redirect_with_default_environment/index.tsx
<#126151 (comment)>:
> +import { ENVIRONMENT_ALL } from '../../../../common/environment_filter_values';
+import { useApmPluginContext } from '../../../context/apm_plugin/use_apm_plugin_context';
+import { useApmRouter } from '../../../hooks/use_apm_router';
+
+export function RedirectWithDefaultEnvironment({
+ children,
+}: {
+ children: React.ReactElement;
+}) {
+ const { core } = useApmPluginContext();
+
+ const apmRouter = useApmRouter();
+ const location = useLocation();
+ const history = useHistory();
+
+ const query = qs.parse(location.search);
What about this?
⬇️ Suggested change
- const query = qs.parse(location.search);
+ const { query } = useApmParams(`*`);
—
Reply to this email directly, view it on GitHub
<#126151 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACWDXERTH5LFO5N3TT6XWDU475BRANCNFSM5PBHJRNA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
💚 Build SucceededMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @dgieselaar |
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
kibanamachine
added
the
backport missing
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
1 similar comment
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
1 similar comment
|
Friendly reminder: Looks like this PR hasn’t been backported yet. |
kibanamachine
removed
the
backport missing
Closes #126091.