[ObsUx] Add feedback form to apm #173758

jennypavlova · 2023-12-20T15:13:59Z

Summary

This PR adds a button to all APM pages navigating to a feedback form. The feedback button component exists in infra so in this PR the component is changed to meet the new requirements and moved to observability_shared. Now the feedback button component supports also prefilling the sanitized path (entry.1876422621). The requirements picked for the sanitized path are described in the issue - it should be for example /app/apm/{page_name} if the user is on the page with the exact path matching /app/apm/{page_name} and /app/apm/{page_name}* for all subpages (for example /app/apm/{page_name}/something/else) - different test scenarios can be found in the unit test: get_path_for_feedback.test.ts

Testing

Go to APM
All APM pages should have a yellow button with the text: Tell us what you think!
Hover on the button to see the prefilled properties and check if every property is prefilled

Click on the form and check if the form is prefilled - The questions that should be prefilled in APM
- Where is your Elastic cluster deployed? (same as infra)
  - By default, the pre-selected option will be on-prem
  - To test the case with the cloud option preselected add xpack.cloud.id: 'elastic_kibana_dev' to your kibana.dev.yaml
  - To test the serverless option run Kibana in serverless mode (observability config)
- What version of Elastic are you using? (same as infra)
- Where in the UI are you?
After checking the APM go to the infra pages and check the feedback button/form

test_apm_feedback.mov

…infra and APM

apmmachine · 2023-12-20T15:14:17Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
/oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

…b.com/jennypavlova/kibana into 172506-obsux-add-feedback-form-to-apm

jennypavlova · 2023-12-20T16:01:15Z

/ci

elasticmachine · 2023-12-21T10:54:08Z

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

crespocarlos

Looks good 👏 ! The form is pre-filled as expected. Didn't test all possible combinations though.

Left a few minor coments.

crespocarlos · 2023-12-21T11:22:09Z

x-pack/plugins/infra/public/pages/metrics/hosts/index.tsx

@@ -29,6 +29,9 @@ const HOSTS_FEEDBACK_LINK =

 export const HostsPage = () => {
  const { isLoading, loadSourceFailureMessage, loadSource, source } = useSourceContext();
+  const {
+    services: { kibanaVersion, isCloudEnv, isServerlessEnv },


Just realized that kibanaVersion,isCloudEnv, and isServerlessEnv are under services. Should we move them out of the services object? They are just flags and not actual services

Do we want to have a separate object here in the context for those values? Should we call it kibanaEnvironment or kibanaSettings maybe? (those flags are about the kibana deployment/version so not sure what the best name is 🤔 wdyt?)

As discussed I picked kibanaEnvironment and added the changes here

crespocarlos · 2023-12-21T11:22:19Z

x-pack/plugins/apm/public/utils/get_path_for_feedback.ts

+  const constructPath = `/${[pathParts[1], pathParts[2], pathParts[3]].join(
+    '/'
+  )}`;


Suggestion: You could use slice here. Should we be worried if pathParts length is less than 3?

Suggested change

const constructPath = `/${[pathParts[1], pathParts[2], pathParts[3]].join(

'/'

)}`;

const constructPath = `/${pathParts.slice(1, 4)].join(

'/'

)}`;

Changed, thanks ✅

crespocarlos · 2023-12-21T11:25:04Z

x-pack/plugins/apm/public/utils/get_path_for_feedback.ts

+};
+
+export const getPathForFeedback = (path: string) => {
+  const pathStartingFromApp = shortenPath(path, '/app');


Is there a safer way to get this /app ? I tried to find some constant or service that would provide this base path, but couldn't find any

I couldn't find it as a constant, I see it is used as a string in other places. I defined a constant here so we can reuse it in case we need to have it somewhere else

crespocarlos · 2023-12-21T11:31:33Z

x-pack/plugins/apm/public/plugin.ts

+  kibanaVersion?: string;
+  isCloudEnv: boolean;
+  isServerlessEnv: boolean;


These are not services, is there any other place we can move these flags into?

They are inside ApmPluginStartDeps only telemetry is in ApmServices. Indeed, they are not service, where do you think we should put them?

Could we pass them directly to renderApp?

…b.com/jennypavlova/kibana into 172506-obsux-add-feedback-form-to-apm

vadimkibana

In the long term we are planning to remove the KibanaReactContext utility, so ideally you would not add new functionality to it. Instead you could create a new React context to pass you KibanaEnvironment state.

export const myEnvContext = React.createContext(/* ... */);

jennypavlova · 2024-01-03T13:52:02Z

In the long term we are planning to remove the KibanaReactContext utility, so ideally you would not add new functionality to it. Instead you could create a new React context to pass you KibanaEnvironment state.
export const myEnvContext = React.createContext(/* ... */);

@vadimkibana Thank you for your review and for letting me know, I didn't know that. I moved the context inside of the plugins. And I reverted the changes inside the KibanaReactContext (as suggested). It's sad that the utility will be removed - do you know if any alternative will be provided? I like the concept of having this shared Kibana context instead of moving it inside the plugins and duplicating part of the logic (Maybe a different shared package or something similar?)

crespocarlos

Hey Jenny. I took another look at it, and I'm just concerned about the stuff we're adding to PluginStartDeps, both in infra (possibly tech debt) and now in APM.

If we compare with the rest of the code base, the start deps are supposed to be used for dependencies with other plugins and these flags don't quite fit in this scenario. Might just be me worrying too much, so I'd like to hear your opinion on this.

crespocarlos · 2024-01-04T09:18:02Z

x-pack/plugins/apm/public/plugin.ts

+            isCloudEnv,
+            isServerlessEnv,
+            kibanaVersion: this.kibanaVersion,


I'm wondering whether these could go into a separate object named kibanaEnv or something like that and decouple them from pluginsStart.

The only difference is that in the renderApp we would add another property to apmPluginContextValue.

The same goes for what we currently have in the infra plugin.

My concern is that we're adding things that are not related to plugin dependencies in the pluginsStart object

Thank you for the comments! IMO the Kibana environment data belongs to the KibanaReactContext as it's something related to the Kibana environment and it's not part of the plugin ( as it is the same for all plugins and it's something that we get as a dependency - same as the services and it should be shared ) But based on the comment this context will be removed so I am not sure if we want to keep the concept of sharing a context (as package/utility) and reuse it. I don't know what will replace the KibanaReactContext but it can be a good place to have this environment-specific data independently there.
So you are right that with the last changes is better to get them out of the pluginsStart. I will do that and I think for now we can keep them in the plugin context until we find a better solution.

So for both infra and apm now I am passing kibanaEnvironment object separately, can you please take a look again :)

crespocarlos · 2024-01-04T09:19:22Z

x-pack/plugins/infra/public/apps/common_providers.tsx

@@ -70,11 +73,15 @@ export const CoreProviders: React.FC<CoreProvidersProps> = ({
    pluginStart
  );

+  const MyEnvContextForPluginProvider = useKibanaEnvironmentContextProvider(plugins);


suggestion: completely up to you

Suggested change

const MyEnvContextForPluginProvider = useKibanaEnvironmentContextProvider(plugins);

const KibanaEnvContextForPluginProvider = useKibanaEnvironmentContextProvider(plugins);

Done, thanks ✅

…d-feedback-form-to-apm

crespocarlos

Looks great! Thanks for making the changes :)

kibana-ci · 2024-01-05T13:08:55Z

💚 Build Succeeded

Buildkite Build
Commit: 9537a30

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`apm`	1543	1546	+3
`infra`	1343	1342	-1
`observabilityShared`	143	144	+1
total			+3

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`observabilityShared`	305	307	+2

Async chunks

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

id	before	after	diff
`apm`	3.1MB	3.1MB	+1.1KB
`infra`	1.3MB	1.3MB	-11.0B
total			+1.1KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`apm`	34.1KB	34.5KB	+413.0B
`infra`	99.0KB	99.5KB	+457.0B
`observabilityShared`	49.9KB	50.8KB	+888.0B
total			+1.7KB

Unknown metric groups

API count

id	before	after	diff
`observabilityShared`	310	312	+2

History

💔 Build #186051 failed 0b4d35e
💔 Build #186034 failed 09d17d5
💔 Build #186028 failed a8c2a007501d3f9ca9c1568ecbde70a6eba41556
💔 Build #185903 failed c576542
💚 Build #185843 succeeded c33379e
💚 Build #185621 succeeded 894d688

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

cc @jennypavlova

Closes elastic#172506 ## Summary This PR adds a button to all APM pages navigating to a feedback form. The feedback button component exists in infra so in this PR the component is changed to meet the new requirements and moved to `observability_shared`. Now the feedback button component supports also prefilling the sanitized path (`entry.1876422621`). The requirements picked for the sanitized path are described in the issue - it should be for example `/app/apm/{page_name}` if the user is on the page with the exact path matching `/app/apm/{page_name}` and `/app/apm/{page_name}*` for all subpages (for example `/app/apm/{page_name}/something/else`) - different test scenarios can be found in the unit test: [get_path_for_feedback.test.ts](https://github.com/elastic/kibana/compare/main...jennypavlova:kibana:172506-obsux-add-feedback-form-to-apm?expand=1#diff-6396807e61353509c44fa38488dfb741549e60f25126024b92596f6b7ac933b8) ## Testing - Go to APM - All APM pages should have a yellow button with the text: `Tell us what you think!` - Hover on the button to see the prefilled properties and check if every property is prefilled <img width="1920" alt="image" src="https://github.com/elastic/kibana/assets/14139027/b57adf04-4e7b-42bb-827f-db554dc4a842"> - Click on the form and check if the form is prefilled - The questions that should be prefilled in APM - Where is your Elastic cluster deployed? (same as infra) - By default, the pre-selected option will be on-prem - To test the case with the cloud option preselected add `xpack.cloud.id: 'elastic_kibana_dev'` to your `kibana.dev.yaml` - To test the serverless option run Kibana in serverless mode (observability config) - What version of Elastic are you using? (same as infra) - Where in the UI are you? - After checking the APM go to the infra pages and check the feedback button/form https://github.com/elastic/kibana/assets/14139027/52cc886f-95a9-4099-b047-93fc64036572

jennypavlova added 4 commits December 13, 2023 17:13

Move FeatureFeedbackButton to obs shared to make it sharable between …

12020b0

…infra and APM

[WIP] APM feedback link

03d4cc1

Add kibana version and cluster type to APM feedback link

d9ff619

Add sanitized path to the feedback url

ab3f1c4

jennypavlova added release_note:skip Skip the PR/issue when compiling release notes apm Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team labels Dec 20, 2023

jennypavlova self-assigned this Dec 20, 2023

jennypavlova added the backport:skip This commit does not require backporting label Dec 20, 2023

jennypavlova added 4 commits December 20, 2023 16:14

Merge branch 'main' into 172506-obsux-add-feedback-form-to-apm

852664c

Update translation key

ce55dd2

Merge branch '172506-obsux-add-feedback-form-to-apm' of https://githu…

f7e15c7

…b.com/jennypavlova/kibana into 172506-obsux-add-feedback-form-to-apm

Merge branch 'main' into 172506-obsux-add-feedback-form-to-apm

deddf67

jennypavlova marked this pull request as ready for review December 21, 2023 10:54

jennypavlova requested review from a team as code owners December 21, 2023 10:54

crespocarlos self-requested a review December 21, 2023 11:32

crespocarlos reviewed Dec 21, 2023

View reviewed changes

jennypavlova added 3 commits December 21, 2023 14:20

CR Changes

c496655

Merge branch '172506-obsux-add-feedback-form-to-apm' of https://githu…

f18945b

…b.com/jennypavlova/kibana into 172506-obsux-add-feedback-form-to-apm

CR change: extract to kibanaEnvironment

aaadb14

jennypavlova requested review from a team as code owners December 21, 2023 15:33

jennypavlova requested a review from crespocarlos December 21, 2023 15:36

vadimkibana reviewed Jan 2, 2024

View reviewed changes

Merge branch 'main' into 172506-obsux-add-feedback-form-to-apm

b3e0d7d

Move KibanaEnvironmentContext to apm and infra

894d688

jennypavlova requested a review from vadimkibana January 3, 2024 14:27

crespocarlos reviewed Jan 4, 2024

View reviewed changes

jennypavlova added 4 commits January 4, 2024 14:30

Merge branch 'main' of github.com:elastic/kibana into 172506-obsux-ad…

c33379e

…d-feedback-form-to-apm

Move kibana env from start deps in APM

b2ad1c3

Move kibana env from start deps in Infra

c13ed03

Merge branch 'main' into 172506-obsux-add-feedback-form-to-apm

c576542

jennypavlova requested a review from crespocarlos January 4, 2024 16:17

jennypavlova added 2 commits January 5, 2024 11:01

Make KibanaEnv optional in common providers

bba1419

Merge branch 'main' of github.com:elastic/kibana into 172506-obsux-ad…

09d17d5

…d-feedback-form-to-apm

jennypavlova force-pushed the 172506-obsux-add-feedback-form-to-apm branch from a8c2a00 to 09d17d5 Compare January 5, 2024 10:01

jennypavlova added 3 commits January 5, 2024 12:01

Make KibanaEnv optional in common providers and fix logs page

b3355d2

Merge branch 'main' into 172506-obsux-add-feedback-form-to-apm

0b4d35e

Merge branch 'main' into 172506-obsux-add-feedback-form-to-apm

9537a30

crespocarlos approved these changes Jan 5, 2024

View reviewed changes

jennypavlova merged commit 35d037b into elastic:main Jan 5, 2024
20 checks passed

jennypavlova deleted the 172506-obsux-add-feedback-form-to-apm branch January 5, 2024 13:52

kibanamachine added the v8.13.0 label Jan 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ObsUx] Add feedback form to apm #173758

[ObsUx] Add feedback form to apm #173758

jennypavlova commented Dec 20, 2023 •

edited

Loading

apmmachine commented Dec 20, 2023

jennypavlova commented Dec 20, 2023

elasticmachine commented Dec 21, 2023

crespocarlos left a comment

crespocarlos Dec 21, 2023

jennypavlova Dec 21, 2023

jennypavlova Dec 21, 2023

crespocarlos Dec 21, 2023

jennypavlova Dec 21, 2023

crespocarlos Dec 21, 2023

jennypavlova Dec 21, 2023

crespocarlos Dec 21, 2023

jennypavlova Dec 21, 2023

crespocarlos Jan 4, 2024

vadimkibana left a comment

jennypavlova commented Jan 3, 2024

crespocarlos left a comment

crespocarlos Jan 4, 2024

jennypavlova Jan 4, 2024

jennypavlova Jan 4, 2024

crespocarlos Jan 4, 2024

jennypavlova Jan 4, 2024

crespocarlos left a comment

kibana-ci commented Jan 5, 2024

API count

	const MyEnvContextForPluginProvider = useKibanaEnvironmentContextProvider(plugins);
	const KibanaEnvContextForPluginProvider = useKibanaEnvironmentContextProvider(plugins);

[ObsUx] Add feedback form to apm #173758

[ObsUx] Add feedback form to apm #173758

Conversation

jennypavlova commented Dec 20, 2023 • edited Loading

Summary

Testing

apmmachine commented Dec 20, 2023

🤖 GitHub comments

jennypavlova commented Dec 20, 2023

elasticmachine commented Dec 21, 2023

crespocarlos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadimkibana left a comment

Choose a reason for hiding this comment

jennypavlova commented Jan 3, 2024

crespocarlos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crespocarlos left a comment

Choose a reason for hiding this comment

kibana-ci commented Jan 5, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

History

jennypavlova commented Dec 20, 2023 •

edited

Loading