-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[ObsUx] Add feedback form to apm #173758
Changes from 8 commits
12020b0
03d4cc1
d9ff619
ab3f1c4
852664c
ce55dd2
f7e15c7
deddf67
c496655
f18945b
aaadb14
b3e0d7d
894d688
c33379e
b2ad1c3
c13ed03
c576542
bba1419
09d17d5
b3355d2
0b4d35e
9537a30
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
|
@@ -104,6 +105,7 @@ export interface ApmPluginSetupDeps { | |
share: SharePluginSetup; | ||
uiActions: UiActionsSetup; | ||
profiling?: ProfilingPluginSetup; | ||
cloud?: CloudSetup; | ||
} | ||
|
||
export interface ApmServices { | ||
|
@@ -137,6 +139,9 @@ export interface ApmPluginStartDeps { | |
observabilityAIAssistant: ObservabilityAIAssistantPluginStart; | ||
dashboard: DashboardStart; | ||
metricsDataAccess: MetricsDataPluginStart; | ||
kibanaVersion?: string; | ||
isCloudEnv: boolean; | ||
isServerlessEnv: boolean; | ||
} | ||
|
||
const servicesTitle = i18n.translate('xpack.apm.navigation.servicesTitle', { | ||
|
@@ -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) { | ||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering whether these could go into a separate object named The only difference is that in the The same goes for what we currently have in the My concern is that we're adding things that are not related to plugin dependencies in the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So for both infra and apm now I am passing |
||
} as ApmPluginStartDeps, | ||
observabilityRuleTypeRegistry, | ||
apmServices: { | ||
telemetry, | ||
|
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); | ||
} | ||
); | ||
}); |
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'); | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a safer way to get this There was a problem hiding this comment. Choose a reason for hiding this commentThe 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( | ||||||||||||||
'/' | ||||||||||||||
)}`; | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: You could use
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed, thanks ✅ |
||||||||||||||
if (pathStartingFromApp === constructPath) { | ||||||||||||||
return pathStartingFromApp; | ||||||||||||||
} | ||||||||||||||
return `${constructPath}*`; | ||||||||||||||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
@@ -29,6 +29,9 @@ const HOSTS_FEEDBACK_LINK = | |
|
||
export const HostsPage = () => { | ||
const { isLoading, loadSourceFailureMessage, loadSource, source } = useSourceContext(); | ||
const { | ||
services: { kibanaVersion, isCloudEnv, isServerlessEnv }, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just realized that There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed I picked |
||
} = useKibanaContextForPlugin(); | ||
|
||
useTrackPageview({ app: 'infra_metrics', path: 'hosts' }); | ||
useTrackPageview({ app: 'infra_metrics', path: 'hosts', delay: 15000 }); | ||
|
@@ -84,6 +87,9 @@ export const HostsPage = () => { | |
<FeatureFeedbackButton | ||
data-test-subj="infraHostsPageTellUsWhatYouThinkButton" | ||
formUrl={HOSTS_FEEDBACK_LINK} | ||
kibanaVersion={kibanaVersion} | ||
isCloudEnv={isCloudEnv} | ||
isServerlessEnv={isServerlessEnv} | ||
/>, | ||
], | ||
}} | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are inside
ApmPluginStartDeps
onlytelemetry
is inApmServices
. Indeed, they are not service, where do you think we should put them?There was a problem hiding this comment.
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
?