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

[ObsUx] Add feedback form to apm #173758

Merged
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
12020b0
Move FeatureFeedbackButton to obs shared to make it sharable between …
jennypavlova Dec 13, 2023
03d4cc1
[WIP] APM feedback link
jennypavlova Dec 18, 2023
d9ff619
Add kibana version and cluster type to APM feedback link
jennypavlova Dec 19, 2023
ab3f1c4
Add sanitized path to the feedback url
jennypavlova Dec 19, 2023
852664c
Merge branch 'main' into 172506-obsux-add-feedback-form-to-apm
jennypavlova Dec 20, 2023
ce55dd2
Update translation key
jennypavlova Dec 20, 2023
f7e15c7
Merge branch '172506-obsux-add-feedback-form-to-apm' of https://githu…
jennypavlova Dec 20, 2023
deddf67
Merge branch 'main' into 172506-obsux-add-feedback-form-to-apm
jennypavlova Dec 20, 2023
c496655
CR Changes
jennypavlova Dec 21, 2023
f18945b
Merge branch '172506-obsux-add-feedback-form-to-apm' of https://githu…
jennypavlova Dec 21, 2023
aaadb14
CR change: extract to kibanaEnvironment
jennypavlova Dec 21, 2023
b3e0d7d
Merge branch 'main' into 172506-obsux-add-feedback-form-to-apm
jennypavlova Jan 3, 2024
894d688
Move KibanaEnvironmentContext to apm and infra
jennypavlova Jan 3, 2024
c33379e
Merge branch 'main' of github.com:elastic/kibana into 172506-obsux-ad…
jennypavlova Jan 4, 2024
b2ad1c3
Move kibana env from start deps in APM
jennypavlova Jan 4, 2024
c13ed03
Move kibana env from start deps in Infra
jennypavlova Jan 4, 2024
c576542
Merge branch 'main' into 172506-obsux-add-feedback-form-to-apm
jennypavlova Jan 4, 2024
bba1419
Make KibanaEnv optional in common providers
jennypavlova Jan 5, 2024
09d17d5
Merge branch 'main' of github.com:elastic/kibana into 172506-obsux-ad…
jennypavlova Jan 5, 2024
b3355d2
Make KibanaEnv optional in common providers and fix logs page
jennypavlova Jan 5, 2024
0b4d35e
Merge branch 'main' into 172506-obsux-add-feedback-form-to-apm
jennypavlova Jan 5, 2024
9537a30
Merge branch 'main' into 172506-obsux-add-feedback-form-to-apm
jennypavlova Jan 5, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import { ObservabilityPageTemplateProps } from '@kbn/observability-shared-plugin
import type { KibanaPageTemplateProps } from '@kbn/shared-ux-page-kibana-template';
import React from 'react';
import { useLocation } from 'react-router-dom';
import { FeatureFeedbackButton } from '@kbn/observability-shared-plugin/public';
import { getPathForFeedback } from '../../../utils/get_path_for_feedback';
import { EnvironmentsContextProvider } from '../../../context/environments_context/environments_context';
import { FETCH_STATUS, useFetcher } from '../../../hooks/use_fetcher';
import { ApmPluginStartDeps } from '../../../plugin';
Expand All @@ -22,6 +24,7 @@ import { useApmPluginContext } from '../../../context/apm_plugin/use_apm_plugin_

// Paths that must skip the no data screen
const bypassNoDataScreenPaths = ['/settings', '/diagnostics'];
const APM_FEEDBACK_LINK = 'https://ela.st/services-feedback';

/*
* This template contains:
Expand Down Expand Up @@ -56,7 +59,15 @@ export function ApmMainTemplate({
const location = useLocation();

const { services } = useKibana<ApmPluginStartDeps>();
const { http, docLinks, observabilityShared, application } = services;
const {
http,
docLinks,
observabilityShared,
application,
kibanaVersion,
isCloudEnv,
isServerlessEnv,
} = services;
const basePath = http?.basePath.get();
const { config } = useApmPluginContext();

Expand All @@ -66,7 +77,7 @@ export function ApmMainTemplate({
return callApmApi('GET /internal/apm/has_data');
}, []);

// create static data view on inital load
// create static data view on initial load
useFetcher(
(callApmApi) => {
const canCreateDataView =
Expand Down Expand Up @@ -111,11 +122,26 @@ export function ApmMainTemplate({
...(showServiceGroupSaveButton ? [<ServiceGroupSaveButton />] : []),
];

const sanitizedPath = getPathForFeedback(window.location.pathname);
const pageHeaderTitle = (
<EuiFlexGroup justifyContent="spaceBetween" wrap={true}>
{pageHeader?.pageTitle ?? pageTitle}
<EuiFlexItem grow={false}>
{environmentFilter && <ApmEnvironmentFilter />}
<EuiFlexGroup justifyContent="center">
<EuiFlexItem grow={false}>
<FeatureFeedbackButton
data-test-subj="infraApmFeedbackLink"
formUrl={APM_FEEDBACK_LINK}
kibanaVersion={kibanaVersion}
isCloudEnv={isCloudEnv}
isServerlessEnv={isServerlessEnv}
sanitizedPath={sanitizedPath}
/>
</EuiFlexItem>
<EuiFlexItem grow={false}>
{environmentFilter && <ApmEnvironmentFilter />}
</EuiFlexItem>
</EuiFlexGroup>
</EuiFlexItem>
</EuiFlexGroup>
);
Expand Down
24 changes: 21 additions & 3 deletions x-pack/plugins/apm/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ import { UsageCollectionStart } from '@kbn/usage-collection-plugin/public';
import { DashboardStart } from '@kbn/dashboard-plugin/public';
import { from } from 'rxjs';
import { map } from 'rxjs/operators';
import type { CloudSetup } from '@kbn/cloud-plugin/public';
import type { ConfigSchema } from '.';
import { registerApmRuleTypes } from './components/alerting/rule_types/register_apm_rule_types';
import {
Expand Down Expand Up @@ -104,6 +105,7 @@ export interface ApmPluginSetupDeps {
share: SharePluginSetup;
uiActions: UiActionsSetup;
profiling?: ProfilingPluginSetup;
cloud?: CloudSetup;
}

export interface ApmServices {
Expand Down Expand Up @@ -137,6 +139,9 @@ export interface ApmPluginStartDeps {
observabilityAIAssistant: ObservabilityAIAssistantPluginStart;
dashboard: DashboardStart;
metricsDataAccess: MetricsDataPluginStart;
kibanaVersion?: string;
isCloudEnv: boolean;
isServerlessEnv: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we pass them directly to renderApp?

}

const servicesTitle = i18n.translate('xpack.apm.navigation.servicesTitle', {
Expand Down Expand Up @@ -187,11 +192,16 @@ const apmTutorialTitle = i18n.translate(

export class ApmPlugin implements Plugin<ApmPluginSetup, ApmPluginStart> {
private telemetry: TelemetryService;
private kibanaVersion: string;
private isServerlessEnv: boolean;
constructor(
private readonly initializerContext: PluginInitializerContext<ConfigSchema>
) {
this.initializerContext = initializerContext;
this.telemetry = new TelemetryService();
this.kibanaVersion = initializerContext.env.packageInfo.version;
this.isServerlessEnv =
initializerContext.env.packageInfo.buildFlavor === 'serverless';
}

public setup(core: CoreSetup, plugins: ApmPluginSetupDeps) {
Expand Down Expand Up @@ -393,18 +403,26 @@ export class ApmPlugin implements Plugin<ApmPluginSetup, ApmPluginStart> {
{ id: 'tutorial', title: apmTutorialTitle, path: '/tutorial' },
],

async mount(appMountParameters: AppMountParameters<unknown>) {
mount: async (appMountParameters: AppMountParameters<unknown>) => {
// Load application bundle and Get start services
const [{ renderApp }, [coreStart, pluginsStart]] = await Promise.all([
import('./application'),
core.getStartServices(),
]);
const isCloudEnv = !!pluginSetupDeps.cloud?.isCloudEnabled;
const isServerlessEnv =
pluginSetupDeps.cloud?.isServerlessEnabled || this.isServerlessEnv;
return renderApp({
coreStart,
pluginsSetup: pluginSetupDeps,
pluginsSetup: pluginSetupDeps as ApmPluginSetupDeps,
appMountParameters,
config,
pluginsStart: pluginsStart as ApmPluginStartDeps,
pluginsStart: {
...pluginsStart,
isCloudEnv,
isServerlessEnv,
kibanaVersion: this.kibanaVersion,
Comment on lines +421 to +423
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} as ApmPluginStartDeps,
observabilityRuleTypeRegistry,
apmServices: {
telemetry,
Expand Down
56 changes: 56 additions & 0 deletions x-pack/plugins/apm/public/utils/get_path_for_feedback.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { getPathForFeedback } from './get_path_for_feedback';

describe('getPathForFeedback ', () => {
const testData = [
{ value: `/ftw/app/apm/traces`, result: '/app/apm/traces' },
{ value: `/app/apm/traces`, result: '/app/apm/traces' },
{
value: `/ftw/app/apm/traces/frontend/transactions/view`,
result: '/app/apm/traces*',
},
{ value: `/app/apm/services`, result: '/app/apm/services' },
{
value: `/longer/path/before/app/apm/services`,
result: '/app/apm/services',
},
{
value: `/ftw/app/apm/services/long/path/after/services`,
result: '/app/apm/services*',
},
{
value: `/ftw/app/apm/dependencies/frontend/transactions/view`,
result: '/app/apm/dependencies*',
},
{ value: `/app/apm/dependencies`, result: '/app/apm/dependencies' },
{
value: `/ftw/app/apm/dependencies/frontend/transactions/view`,
result: '/app/apm/dependencies*',
},
{
value: `/ftw/app/apm/settings/frontend/transactions/view`,
result: '/app/apm/settings*',
},
{
value: `/app/apm/some-page/frontend/transactions/view`,
result: '/app/apm/some-page*',
},
{
value: `/app/apm/settings`,
result: '/app/apm/settings',
},
];

it.each(testData)(
'Returns correct path for the feedback form $value -> $result',
({ value, result }) => {
expect(getPathForFeedback(value)).toBe(result);
}
);
});
26 changes: 26 additions & 0 deletions x-pack/plugins/apm/public/utils/get_path_for_feedback.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

export const shortenPath = (path: string, pathStart: string) => {
if (path.startsWith(pathStart)) {
return path;
}
const indexOfPathStart = path.indexOf(pathStart);
return path.substring(indexOfPathStart);
};

export const getPathForFeedback = (path: string) => {
const pathStartingFromApp = shortenPath(path, '/app');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const pathParts = pathStartingFromApp.split('/');
const constructPath = `/${[pathParts[1], pathParts[2], pathParts[3]].join(
'/'
)}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(
'/'
)}`;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed, thanks ✅

if (pathStartingFromApp === constructPath) {
return pathStartingFromApp;
}
return `${constructPath}*`;
};
10 changes: 8 additions & 2 deletions x-pack/plugins/infra/public/pages/metrics/hosts/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@

import { EuiErrorBoundary } from '@elastic/eui';
import React from 'react';
import { useTrackPageview } from '@kbn/observability-shared-plugin/public';
import { useTrackPageview, FeatureFeedbackButton } from '@kbn/observability-shared-plugin/public';
import { APP_WRAPPER_CLASS } from '@kbn/core/public';
import { css } from '@emotion/react';
import { i18n } from '@kbn/i18n';
import { FeatureFeedbackButton } from '../../../components/feature_feedback_button';
import { useKibanaContextForPlugin } from '../../../hooks/use_kibana';
import { SourceErrorPage } from '../../../components/source_error_page';
import { SourceLoadingPage } from '../../../components/source_loading_page';
import { useSourceContext } from '../../../containers/metrics_source';
Expand All @@ -29,6 +29,9 @@ const HOSTS_FEEDBACK_LINK =

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed I picked kibanaEnvironment and added the changes here

} = useKibanaContextForPlugin();

useTrackPageview({ app: 'infra_metrics', path: 'hosts' });
useTrackPageview({ app: 'infra_metrics', path: 'hosts', delay: 15000 });
Expand Down Expand Up @@ -84,6 +87,9 @@ export const HostsPage = () => {
<FeatureFeedbackButton
data-test-subj="infraHostsPageTellUsWhatYouThinkButton"
formUrl={HOSTS_FEEDBACK_LINK}
kibanaVersion={kibanaVersion}
isCloudEnv={isCloudEnv}
isServerlessEnv={isServerlessEnv}
/>,
],
}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,27 @@ import { EuiFlexGroup, EuiFlexItem, EuiGlobalToastList } from '@elastic/eui';
import React from 'react';
import { FormattedMessage } from '@kbn/i18n-react';
import useLocalStorage from 'react-use/lib/useLocalStorage';
import { FeatureFeedbackButton } from '../../../../components/feature_feedback_button';
import { FeatureFeedbackButton } from '@kbn/observability-shared-plugin/public';
import { useKibanaContextForPlugin } from '../../../../hooks/use_kibana';

const KUBERNETES_TOAST_STORAGE_KEY = 'kubernetesToastKey';
const KUBERNETES_FEEDBACK_LINK = 'https://ela.st/k8s-feedback';

export const SurveyKubernetes = () => {
const [isToastSeen, setIsToastSeen] = useLocalStorage(KUBERNETES_TOAST_STORAGE_KEY, false);
const markToastAsSeen = () => setIsToastSeen(true);
const {
services: { kibanaVersion, isCloudEnv, isServerlessEnv },
} = useKibanaContextForPlugin();

return (
<>
<FeatureFeedbackButton
formUrl={KUBERNETES_FEEDBACK_LINK}
data-test-subj="infra-kubernetes-feedback-link"
kibanaVersion={kibanaVersion}
isCloudEnv={isCloudEnv}
isServerlessEnv={isServerlessEnv}
surveyButtonText={
<FormattedMessage
id="xpack.infra.homePage.tellUsWhatYouThinkK8sLink"
Expand Down Expand Up @@ -60,6 +67,9 @@ export const SurveyKubernetes = () => {
data-test-subj="infra-toast-kubernetes-survey-start"
onClickCapture={markToastAsSeen}
defaultButton={true}
kibanaVersion={kibanaVersion}
isCloudEnv={isCloudEnv}
isServerlessEnv={isServerlessEnv}
surveyButtonText={
<FormattedMessage
id="xpack.infra.homePage.kubernetesToastButton"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
*/

import React from 'react';
import { FeatureFeedbackButton } from '../../../../components/feature_feedback_button';
import { FeatureFeedbackButton } from '@kbn/observability-shared-plugin/public';
import { useKibanaContextForPlugin } from '../../../../hooks/use_kibana';

import { useWaffleOptionsContext } from '../hooks/use_waffle_options';
import { SurveyKubernetes } from './survey_kubernetes';
Expand All @@ -15,6 +16,9 @@ const INVENTORY_FEEDBACK_LINK = 'https://ela.st/survey-infra-inventory?usp=pp_ur

export const SurveySection = () => {
const { nodeType } = useWaffleOptionsContext();
const {
services: { kibanaVersion, isCloudEnv, isServerlessEnv },
} = useKibanaContextForPlugin();

return (
<>
Expand All @@ -24,6 +28,9 @@ export const SurveySection = () => {
<FeatureFeedbackButton
data-test-subj="infraInventoryFeedbackLink"
formUrl={INVENTORY_FEEDBACK_LINK}
kibanaVersion={kibanaVersion}
isCloudEnv={isCloudEnv}
isServerlessEnv={isServerlessEnv}
/>
)}
</>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
import { EuiErrorBoundary } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import React, { useEffect, useState } from 'react';
import { useTrackPageview } from '@kbn/observability-shared-plugin/public';
import { FeatureFeedbackButton } from '../../../components/feature_feedback_button';
import { useTrackPageview, FeatureFeedbackButton } from '@kbn/observability-shared-plugin/public';
import { useKibanaContextForPlugin } from '../../../hooks/use_kibana';
import { SourceLoadingPage } from '../../../components/source_loading_page';
import { useMetricsExplorerViews } from '../../../hooks/use_metrics_explorer_views';
import { MetricsSourceConfigurationProperties } from '../../../../common/metrics_sources';
Expand Down Expand Up @@ -52,6 +52,9 @@ export const MetricsExplorerPage = ({ source, derivedIndexPattern }: MetricsExpl
} = useMetricsExplorerState(source, derivedIndexPattern, enabled);
const { currentView } = useMetricsExplorerViews();
const { source: sourceContext, metricIndicesExist } = useSourceContext();
const {
services: { kibanaVersion, isCloudEnv, isServerlessEnv },
} = useKibanaContextForPlugin();

useTrackPageview({ app: 'infra_metrics', path: 'metrics_explorer' });
useTrackPageview({ app: 'infra_metrics', path: 'metrics_explorer', delay: 15000 });
Expand Down Expand Up @@ -100,6 +103,9 @@ export const MetricsExplorerPage = ({ source, derivedIndexPattern }: MetricsExpl
<FeatureFeedbackButton
formUrl={METRICS_EXPLORER_FEEDBACK_URL}
data-test-subj="infraMetricsExplorerFeedbackLink"
kibanaVersion={kibanaVersion}
isCloudEnv={isCloudEnv}
isServerlessEnv={isServerlessEnv}
/>,
],
}}
Expand Down
Loading