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.

Sign up for GitHub

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

add feature flags to apm config and serverless.oblt.yml #159136

Merged
merged 11 commits into from
Jun 19, 2023
Merged

add feature flags to apm config and serverless.oblt.yml #159136

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
35422ce
add feature flags to apm config and serverless.oblt.yml
MiriamAparicio Jun 6, 2023
f40dd45
Merge remote-tracking branch 'upstream/main' into 159040-add-config-f…
MiriamAparicio Jun 12, 2023
cc554c2
fix incorrent feature flag value
MiriamAparicio Jun 12, 2023
3c8b968
fix plugin functional tests
MiriamAparicio Jun 12, 2023
97c1182
Merge remote-tracking branch 'upstream/main' into 159040-add-config-f…
MiriamAparicio Jun 13, 2023
eca126c
remove schemaAvailable flag
MiriamAparicio Jun 13, 2023
33ad69e
remove spacesAvailable flag
MiriamAparicio Jun 13, 2023
42fb17a
improve readability in config for feature flags
MiriamAparicio Jun 14, 2023
fd927d2
fix merge conflicts
MiriamAparicio Jun 18, 2023
85c56fe
Merge branch 'main' into 159040-add-config-feature-flags
MiriamAparicio Jun 19, 2023
2091bf7
Merge branch 'main' into 159040-add-config-feature-flags
MiriamAparicio Jun 19, 2023
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
8 changes: 8 additions & 0 deletions config/serverless.oblt.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,11 @@ xpack.apm.serverlessOnboarding: true
xpack.fleet.packages:
- name: apm
version: latest
## Disable APM UI components and API calls
xpack.apm.featureFlags.agentConfigurationAvailable: false
xpack.apm.featureFlags.configurableIndicesAvailable: false
xpack.apm.featureFlags.infrastructureTabAvailable: false
xpack.apm.featureFlags.infraUiAvailable: false
xpack.apm.featureFlags.migrationToFleetAvailable: false
xpack.apm.featureFlags.sourcemapApiAvailable: false
xpack.apm.featureFlags.storageExplorerAvailable: false
Comment on lines +30 to +37
Copy link
Member

@afharo afharo Jun 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

NIT: we could also use the YAML tree format to avoid repetition 😇

Suggested change
## Disable APM UI components and API calls
xpack.apm.featureFlags.agentConfigurationAvailable: false
xpack.apm.featureFlags.configurableIndicesAvailable: false
xpack.apm.featureFlags.infrastructureTabAvailable: false
xpack.apm.featureFlags.infraUiAvailable: false
xpack.apm.featureFlags.migrationToFleetAvailable: false
xpack.apm.featureFlags.sourcemapApiAvailable: false
xpack.apm.featureFlags.storageExplorerAvailable: false
## Disable APM UI components and API calls
xpack.apm.featureFlags:
agentConfigurationAvailable: false
configurableIndicesAvailable: false
infrastructureTabAvailable: false
infraUiAvailable: false
migrationToFleetAvailable: false
sourcemapApiAvailable: false
storageExplorerAvailable: false

7 changes: 7 additions & 0 deletions test/plugin_functional/test_suites/core_plugins/rendering.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,13 @@ export default function ({ getService }: PluginFunctionalProviderContext) {
'xpack.apm.managedServiceUrl (any)',
'xpack.apm.serverlessOnboarding (any)',
'xpack.apm.latestAgentVersionsUrl (string)',
'xpack.apm.featureFlags.agentConfigurationAvailable (any)',
'xpack.apm.featureFlags.configurableIndicesAvailable (any)',
'xpack.apm.featureFlags.infrastructureTabAvailable (any)',
'xpack.apm.featureFlags.infraUiAvailable (any)',
'xpack.apm.featureFlags.migrationToFleetAvailable (any)',
'xpack.apm.featureFlags.sourcemapApiAvailable (any)',
'xpack.apm.featureFlags.storageExplorerAvailable (any)',
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these should be booleans

Copy link
Contributor Author

@MiriamAparicio MiriamAparicio Jun 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

hi @thomheymann,
I set them initially to boolean in this commit bf0a260, but the test were failing
image

Copy link
Contributor

Choose a reason for hiding this comment

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

That's strange. Maybe the renderer doesn't infer the type correctly because of the context conditional:
https://github.com/elastic/kibana/pull/159136/files#diff-fa8fe01d402a7eb5cbc387759a4ea3930f3cc056d90477c04232d3ab91afaac7R17-R25
Let's leave as is for now in that case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes if we use a conditional rendering, the renderer is currently unable to infer the type based on provided default value. Hence it will always result in any type

'xpack.cases.files.allowedMimeTypes (array)',
'xpack.cases.files.maxSize (number)',
'xpack.cases.markdownPlugins.lens (boolean)',
Expand Down
10 changes: 0 additions & 10 deletions x-pack/plugins/apm/common/apm_feature_flags.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ export enum ApmFeatureFlagName {
InfraUiAvailable = 'infraUiAvailable',
MigrationToFleetAvailable = 'migrationToFleetAvailable',
SourcemapApiAvailable = 'sourcemapApiAvailable',
SpacesAvailable = 'spacesAvailable',
SchemaAvailable = 'schemaAvailable',
StorageExplorerAvailable = 'storageExplorerAvailable',
}

Expand Down Expand Up @@ -45,14 +43,6 @@ const apmFeatureFlagMap = {
default: true,
type: t.boolean,
},
[ApmFeatureFlagName.SpacesAvailable]: {
default: true,
type: t.boolean,
},
[ApmFeatureFlagName.SchemaAvailable]: {
default: true,
type: t.boolean,
},
[ApmFeatureFlagName.StorageExplorerAvailable]: {
default: true,
type: t.boolean,
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/apm/public/components/routing/templates/apm_service_template/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,6 @@ function useTabs({ selectedTab }: { selectedTab: Tab['key'] }) {
);

const router = useApmRouter();

const isInfraTabAvailable = useApmFeatureFlag(
ApmFeatureFlagName.InfrastructureTabAvailable
);
Expand Down
14 changes: 13 additions & 1 deletion x-pack/plugins/apm/public/components/routing/templates/settings_template.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,13 @@ const coreMock = {
},
} as unknown as Partial<CoreStart>;

const configMock = {
featureFlags: {
agentConfigurationAvailable: true,
configurableIndicesAvailable: true,
},
};

const stories: Meta<Args> = {
title: 'routing/templates/SettingsTemplate',
component: SettingsTemplate,
Expand All @@ -36,7 +43,12 @@ const stories: Meta<Args> = {

return (
<MockApmPluginStorybook
apmContext={{ core: coreMock } as unknown as ApmPluginContextValue}
apmContext={
{
core: coreMock,
config: configMock,
} as unknown as ApmPluginContextValue
}
>
<StoryComponent />
</MockApmPluginStorybook>
Expand Down
10 changes: 5 additions & 5 deletions x-pack/plugins/apm/public/components/routing/templates/settings_template.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export function SettingsTemplate({ children, selectedTab }: Props) {
const agentConfigurationAvailable = useApmFeatureFlag(
ApmFeatureFlagName.AgentConfigurationAvailable
);
const schemaTabAvailable = useApmFeatureFlag(
const migrationToFleetAvailable = useApmFeatureFlag(
ApmFeatureFlagName.MigrationToFleetAvailable
);
const indicesAvailable = useApmFeatureFlag(
Expand All @@ -59,7 +59,7 @@ export function SettingsTemplate({ children, selectedTab }: Props) {
router,
defaultEnvironment,
agentConfigurationAvailable,
schemaTabAvailable,
migrationToFleetAvailable,
indicesAvailable,
});

Expand All @@ -84,15 +84,15 @@ function getTabs({
router,
defaultEnvironment,
agentConfigurationAvailable,
schemaTabAvailable,
migrationToFleetAvailable,
indicesAvailable,
}: {
core: CoreStart;
selectedTab: Tab['key'];
router: ApmRouter;
defaultEnvironment: Environment;
agentConfigurationAvailable: boolean;
schemaTabAvailable: boolean;
migrationToFleetAvailable: boolean;
indicesAvailable: boolean;
}) {
const canReadMlJobs = !!core.application.capabilities.ml?.canGetJobs;
Expand Down Expand Up @@ -171,7 +171,7 @@ function getTabs({
]
: []),

...(schemaTabAvailable
...(migrationToFleetAvailable
? [
{
key: 'schema' as const,
Expand Down
9 changes: 9 additions & 0 deletions x-pack/plugins/apm/public/context/apm_plugin/mock_apm_plugin_context.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,15 @@ const mockConfig: ConfigSchema = {
latestAgentVersionsUrl: '',
serverlessOnboarding: false,
managedServiceUrl: '',
featureFlags: {
agentConfigurationAvailable: true,
configurableIndicesAvailable: true,
infrastructureTabAvailable: true,
infraUiAvailable: true,
migrationToFleetAvailable: true,
sourcemapApiAvailable: true,
storageExplorerAvailable: true,
},
};

const urlService = new UrlService({
Expand Down
11 changes: 3 additions & 8 deletions x-pack/plugins/apm/public/hooks/use_apm_feature_flag.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,17 @@
* 2.0.
*/

import { useMemo } from 'react';
import {
ApmFeatureFlagName,
getApmFeatureFlags,
ValueOfApmFeatureFlag,
} from '../../common/apm_feature_flags';
import { useApmPluginContext } from '../context/apm_plugin/use_apm_plugin_context';

export function useApmFeatureFlag<
TApmFeatureFlagName extends ApmFeatureFlagName
>(
featureFlag: TApmFeatureFlagName
): ValueOfApmFeatureFlag<TApmFeatureFlagName> {
const featureFlags = useMemo(() => {
// this should be replaced with an API call
return getApmFeatureFlags();
}, []);

return featureFlags[featureFlag];
const { config } = useApmPluginContext();
return config.featureFlags[featureFlag];
}
9 changes: 9 additions & 0 deletions x-pack/plugins/apm/public/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,15 @@ export interface ConfigSchema {
latestAgentVersionsUrl: string;
serverlessOnboarding: boolean;
managedServiceUrl: string;
featureFlags: {
agentConfigurationAvailable: boolean;
configurableIndicesAvailable: boolean;
infrastructureTabAvailable: boolean;
infraUiAvailable: boolean;
migrationToFleetAvailable: boolean;
sourcemapApiAvailable: boolean;
storageExplorerAvailable: boolean;
};
}

export const plugin: PluginInitializer<ApmPluginSetup, ApmPluginStart> = (
Expand Down
59 changes: 59 additions & 0 deletions x-pack/plugins/apm/server/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,64 @@ const configSchema = schema.object({
schema.string({ defaultValue: '' }),
schema.never()
),
featureFlags: schema.object({
achyutjhunjhunwala marked this conversation as resolved.
Show resolved Hide resolved
agentConfigurationAvailable: schema.conditional(
schema.contextRef('serverless'),
true,
schema.boolean({
defaultValue: false,
}),
schema.oneOf([schema.literal(true)], { defaultValue: true })
Copy link
Contributor

@achyutjhunjhunwala achyutjhunjhunwala Jun 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I didn't understand the notEqual logic here.

Basically, if we are not running in serverless context, its being enforced to a true literal check and the default value could be true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's like and if/else

Copy link
Contributor

@achyutjhunjhunwala achyutjhunjhunwala Jun 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, i understand the schema.conditional.
What i don't get it is, when we are not running in serverless mode, you always want this settings to be true ? This is what i don't get it.

This config means

If we are running in serverless mode, then default value is false, but it would give *.yml file the preference. Which mean in serverless.oblt.yml file, it needs to be set as true only then it will be enabled.

If we are not running in serverless mode, it will always be true.

is this the expected behaviour ?

Copy link
Contributor

@achyutjhunjhunwala achyutjhunjhunwala Jun 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Reason for asking this is, since you are setting it as true in non serverless mode, you will have to update the security teams tests also where they check for exposed settings.

This might also need documentation update for these exposed settings. The moment you update the security tests, someone from the security team will provide more details

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we are not running in serverless mode, it will always be true.

is this the expected behaviour ?

Yes, we want these values to be true when we are not in serverless

This might also need documentation update for these exposed settings. The moment you update the security tests, someone from the security team will provide more details

As you said I needed to update the security test and the team was ping to review these changes, I'm waiting for them for more details

),
configurableIndicesAvailable: schema.conditional(
schema.contextRef('serverless'),
true,
schema.boolean({
defaultValue: false,
}),
schema.oneOf([schema.literal(true)], { defaultValue: true })
),
MiriamAparicio marked this conversation as resolved.
Show resolved Hide resolved
infrastructureTabAvailable: schema.conditional(
schema.contextRef('serverless'),
true,
schema.boolean({
defaultValue: false,
}),
schema.oneOf([schema.literal(true)], { defaultValue: true })
),
infraUiAvailable: schema.conditional(
schema.contextRef('serverless'),
true,
schema.boolean({
defaultValue: false,
}),
schema.oneOf([schema.literal(true)], { defaultValue: true })
),
migrationToFleetAvailable: schema.conditional(
schema.contextRef('serverless'),
true,
schema.boolean({
defaultValue: false,
}),
schema.oneOf([schema.literal(true)], { defaultValue: true })
),
sourcemapApiAvailable: schema.conditional(
schema.contextRef('serverless'),
true,
schema.boolean({
defaultValue: false,
}),
schema.oneOf([schema.literal(true)], { defaultValue: true })
),
storageExplorerAvailable: schema.conditional(
schema.contextRef('serverless'),
true,
schema.boolean({
defaultValue: false,
}),
schema.oneOf([schema.literal(true)], { defaultValue: true })
),
}),
});

// plugin config
Expand Down Expand Up @@ -129,6 +187,7 @@ export const config: PluginConfigDescriptor<APMConfig> = {
latestAgentVersionsUrl: true,
managedServiceUrl: true,
serverlessOnboarding: true,
featureFlags: true,
},
schema: configSchema,
};
Expand Down
3 changes: 1 addition & 2 deletions x-pack/plugins/apm/server/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ import { migrateLegacyAPMIndicesToSpaceAware } from './saved_objects/migrations/
import { scheduleSourceMapMigration } from './routes/source_maps/schedule_source_map_migration';
import { createApmSourceMapIndexTemplate } from './routes/source_maps/create_apm_source_map_index_template';
import { addApiKeysToEveryPackagePolicyIfMissing } from './routes/fleet/api_keys/add_api_keys_to_policies_if_missing';
import { getApmFeatureFlags } from '../common/apm_feature_flags';
import { apmTutorialCustomIntegration } from '../common/tutorial/tutorials';

export class APMPlugin
Expand Down Expand Up @@ -183,7 +182,7 @@ export class APMPlugin
},
logger: this.logger,
config: currentConfig,
featureFlags: getApmFeatureFlags(),
featureFlags: currentConfig.featureFlags,
repository: getGlobalApmServerRouteRepository(),
ruleDataClient,
plugins: resourcePlugins,
Expand Down