-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
add feature flags to apm config and serverless.oblt.yml #159136
add feature flags to apm config and serverless.oblt.yml #159136
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
e83afd8
to
f40dd45
Compare
Pinging @elastic/apm-ui (Team:APM) |
x-pack/plugins/apm/public/components/routing/templates/settings_template.stories.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/server/index.ts
Outdated
schema.boolean({ | ||
defaultValue: false, | ||
}), | ||
schema.oneOf([schema.literal(true)], { defaultValue: true }) |
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.
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
?
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.
Yes, it's like and if/else
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.
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 ?
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.
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
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.
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
748888b
to
bf0a260
Compare
bf0a260
to
3c8b968
Compare
e7a0959
to
eca126c
Compare
'xpack.apm.featureFlags.infraUiAvailable (any)', | ||
'xpack.apm.featureFlags.migrationToFleetAvailable (any)', | ||
'xpack.apm.featureFlags.sourcemapApiAvailable (any)', | ||
'xpack.apm.featureFlags.storageExplorerAvailable (any)', |
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.
I think these should be booleans
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.
hi @thomheymann,
I set them initially to boolean in this commit bf0a260, but the test were failing
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.
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.
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.
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
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.
LGTM!
1ef4e1c
to
fd927d2
Compare
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.
lgtm
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.
LGTM 👍🏼 Good work with the flags
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.
config/serverless.oblt.yml
changes LGTM
## 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 |
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.
NIT: we could also use the YAML tree format to avoid repetition 😇
## 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 |
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Async chunks
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Closes #159040
Add configuration values to hide UI components and block api in serverless.oblt.yml
Examples
Non Serverless
Serverless