[APM] Explicitly set environment for cross-service links by dgieselaar · Pull Request #87481 · elastic/kibana

dgieselaar · 2021-01-06T13:41:34Z

Closes #87028.

Closes elastic#87028.

elasticmachine · 2021-01-06T13:41:36Z

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

dgieselaar · 2021-01-06T15:41:26Z

@elasticmachine merge upstream

x-pack/plugins/apm/common/environment_filter_values.ts

sorenlouv

lgtm. I expect we'll remove the logic in isEnvironmentEqualToQueryEnvironment in 7.12 as we remove the "all" option?

…ross-service

…ana into environment-cross-service

dgieselaar · 2021-01-11T13:36:37Z

@sqren I made some changes, maybe good to have another look

…ross-service

sorenlouv · 2021-01-12T23:24:00Z

x-pack/plugins/apm/common/environment_filter_values.ts

+  requestedEnvironment?: string,
+  queryEnvironment?: string


Perhaps:

Suggested change

requestedEnvironment?: string,

queryEnvironment?: string

requestedEnvironment?: string,

currentEnvironment?: string

I wanted to keep this as queryEnvironment because how an empty value is interpreted is specific to whether it's a query parameter. Maybe currentQueryEnvironment is a good compromise here, but happy to go with your suggestion as well.

sorenlouv · 2021-01-12T23:51:19Z

x-pack/plugins/apm/common/environment_filter_values.ts

+) {
+  const normalizedRequestedEnvironment =
+    requestedEnvironment || ENVIRONMENT_NOT_DEFINED.value;
+  const normalizedQueryEnvironment = queryEnvironment || ENVIRONMENT_ALL.value;


I'm wondering if we should change the default so that if no environment is set (in the url) it'll default to ENVIRONMENT_NOT_DEFINED.value.

Ideally when empty I think it should pick "production" if that's available or the top environment. This is similar to what we do with transaction type.

Hmm, I think all is a good default here - there's no all option for transaction types so maybe not the right thing to compare it to.

sorenlouv

Just a few comments. Feel free to merge as-is

sorenlouv · 2021-01-14T10:48:01Z

x-pack/plugins/apm/common/environment_filter_values.ts

  return environmentLabels[environment] || environment;
 }
+
+export function getEnvironmentUrlParam(


Can you add a comment why this helper is needed?

sorenlouv · 2021-01-14T10:48:05Z

...ns/apm/public/components/app/TransactionDetails/WaterfallWithSummmary/MaybeViewTraceLink.tsx

+    const nextEnvironment = getEnvironmentUrlParam(
+      rootTransaction?.service.environment,
+      environment
+    );


This would be easier to understand if it was destructured:

Suggested change

const nextEnvironment = getEnvironmentUrlParam(

rootTransaction?.service.environment,

environment

);

const nextEnvironment = getEnvironmentUrlParam({

requestedEnvironment: rootTransaction?.service.environment,

currentEnvironment: environment

});

sorenlouv · 2021-01-14T10:50:04Z

x-pack/plugins/apm/public/components/shared/Links/apm/ServiceMapLink.tsx

    ? `/services/${serviceName}/service-map`
    : '/service-map';
-  return useAPMHref(pathFor);
+  return useAPMHref({ path: pathFor });


Can you rename pathFor to path or inline it?

Suggested change

return useAPMHref({ path: pathFor });

return useAPMHref({ path });

sorenlouv · 2021-01-14T11:05:13Z

x-pack/plugins/apm/public/components/shared/Links/apm/APMLink.tsx

+  };

-  return getAPMHref({ basePath, path, query, search });
+  return getAPMHref({ basePath, path, query: nextQuery, search });


Good idea destructuring useAPMHref 👍

As I mentioned to @cauemarcondes in another PR, I don't think getAPMHref should take both query and search (the latter is essentially just a serialised version of the former). But that can wait until 7.12.

…ross-service

kibanamachine · 2021-01-15T11:16:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: dc856db

Metrics [docs]

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.5MB	5.5MB	+2.3KB

History

💚 Build #98825 succeeded b2dd317
💚 Build #98814 succeeded cb45ec7
💚 Build #98189 succeeded 5db70d8
💚 Build #97846 succeeded 7a164a6
💚 Build #97320 succeeded 7e9ac24

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>

* master: (33 commits) [Security Solution][Case] Fix patch cases integration test with alerts (elastic#88311) [Security Solutions][Detection Engine] Removes duplicate API calls (elastic#88420) Fix log msg (elastic#88370) [Test] Add tag cloud visualization to dashboard in functional test for reporting (elastic#87600) removing kibana-core-ui from codeowners (elastic#88111) [Alerting] Migrate Event Log plugin to TS project references (elastic#81557) [Maps] fix zooming while drawing shape filter logs errors in console (elastic#88413) Porting fixes 1 (elastic#88477) [APM] Explicitly set environment for cross-service links (elastic#87481) chore(NA): remove mocha junit ci integrations (elastic#88129) [APM] Only display relevant sections for rum agent in service overview (elastic#88410) [Enterprise Search] Automatically mock shared logic files (elastic#88494) [APM] Disable Create custom link button on Transaction details page for read-only users [Docs] clean-up vega map reference documenation (elastic#88487) [Security Solution] Fix Timeline event details layout (elastic#88377) Change DELETE to POST for _bulk_delete to avoid incompatibility issues (elastic#87914) [Monitoring] Change cloud messaging on no data page (elastic#88375) [Uptime] clear ping state when PingList component in unmounted (elastic#88321) [APM] Consistent terminology for latency and throughput (elastic#88452) fix copy (elastic#88481) ...

[APM] Explicitly set environment for cross-service links if needed

86b4fa3

Closes elastic#87028.

dgieselaar added release_note:fix Team:APM - DEPRECATED Use Team:obs-ux-infra_services. v7.11.0 labels Jan 6, 2021

dgieselaar requested a review from a team January 6, 2021 13:41

Merge branch 'master' into environment-cross-service

7e9ac24

sorenlouv reviewed Jan 6, 2021

View reviewed changes

x-pack/plugins/apm/common/environment_filter_values.ts Show resolved Hide resolved

sorenlouv approved these changes Jan 6, 2021

View reviewed changes

dgieselaar added 3 commits January 11, 2021 11:38

Merge branch 'master' of github.com:elastic/kibana into environment-c…

8ac7026

…ross-service

Merge branch 'master' of github.com:elastic/kibana into environment-c…

2fe526b

…ross-service

Merge branch 'environment-cross-service' of github.com:dgieselaar/kib…

7a164a6

…ana into environment-cross-service

dgieselaar requested a review from sorenlouv January 11, 2021 13:35

Merge branch 'master' of github.com:elastic/kibana into environment-c…

5db70d8

…ross-service

sorenlouv reviewed Jan 12, 2021

View reviewed changes

sorenlouv approved these changes Jan 12, 2021

View reviewed changes

sorenlouv reviewed Jan 14, 2021

View reviewed changes

dgieselaar added 4 commits January 14, 2021 12:27

Merge branch 'master' of github.com:elastic/kibana into environment-c…

cb45ec7

…ross-service

Review feedback

b2dd317

Merge branch 'master' of github.com:elastic/kibana into environment-c…

b55523c

…ross-service

Review feedback

dc856db

dgieselaar changed the title ~~[APM] Explicitly set environment for cross-service links if needed~~ [APM] Explicitly set environment for cross-service links Jan 15, 2021

dgieselaar merged commit f5ec1dc into elastic:master Jan 15, 2021

dgieselaar deleted the environment-cross-service branch January 15, 2021 21:34

dgieselaar mentioned this pull request Jan 15, 2021

[7.x] [APM] Explicitly set environment for links (#87481) #88537

Merged

dgieselaar added a commit to dgieselaar/kibana that referenced this pull request Jan 15, 2021

[APM] Explicitly set environment for cross-service links (elastic#87481)

c1ce060

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

dgieselaar added a commit to dgieselaar/kibana that referenced this pull request Jan 15, 2021

[APM] Explicitly set environment for cross-service links (elastic#87481)

582dfad

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

dgieselaar mentioned this pull request Jan 15, 2021

[7.11] [APM] Explicitly set environment for links (#87481) #88538

Merged

	return useAPMHref({ path: pathFor });
	return useAPMHref({ path });

Conversation

dgieselaar commented Jan 6, 2021

Uh oh!

elasticmachine commented Jan 6, 2021

Uh oh!

dgieselaar commented Jan 6, 2021

Uh oh!

Uh oh!

sorenlouv left a comment

Choose a reason for hiding this comment

Uh oh!

dgieselaar commented Jan 11, 2021

Uh oh!

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

dgieselaar Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

sorenlouv Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

dgieselaar Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

sorenlouv left a comment

Choose a reason for hiding this comment

Uh oh!

sorenlouv Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

sorenlouv Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

sorenlouv Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

sorenlouv Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jan 15, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sorenlouv Jan 12, 2021 •

edited

Loading