-
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
[On-Week] Hot update of APM/EBT labels #157093
Merged
Merged
Changes from 43 commits
Commits
Show all changes
50 commits
Select commit
Hold shift + click to select a range
62c9016
[On-Week] Hot update of APM/EBT labels
afharo e58aa4e
[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…
kibanamachine 7644cfa
Use custom apm-agent-nodejs https://github.com/elastic/apm-agent-node…
afharo 17b34e9
Merge branch 'onweek/hot-update-of-apm-labels' of github.com:afharo/k…
afharo 4b94138
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine da5a9b0
Add new method to APM mocks
afharo be56393
Merge branch 'onweek/hot-update-of-apm-labels' of github.com:afharo/k…
afharo e031ed0
Spread the update to the browser
afharo 1c1062b
Spread initial config to the APM global labels
afharo 1cc38fb
Spread the labels updates to EBT context
afharo 4ba34c1
update journey tearUp hook
dmlemeshko 02b0967
Allow unauthenticated requests to telemetry/config API
afharo b7370e0
Merge branch 'onweek/hot-update-of-apm-labels' of github.com:afharo/k…
afharo dea544b
add wait and move api call
dmlemeshko 7f7fc21
Merge remote-tracking branch 'upstream/main' into onweek/hot-update-o…
dmlemeshko d1adabc
Introduce the concept of dynamic configuration in the core Config Ser…
afharo 41b9065
[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…
kibanamachine 464aa4f
Add/fix tests
afharo d45ea1b
Small improvements after self-review
afharo 7f05ea9
Fix ConfigService mock
afharo 8069850
Update exposed configs to anonymous pages
afharo f956a6a
Export missing type from the index
afharo d328290
Merge branch 'main' into onweek/hot-update-of-apm-labels
kibanamachine 0b90a33
Merge branch 'main' of github.com:elastic/kibana into onweek/hot-upda…
afharo fb54e70
Address initial feedback
afharo 670f9f5
Do not swallow errors
afharo 7415208
Use `main` from apm-nodejs-agent until released
afharo be0084d
Update access tag
afharo 7d2fff6
Merge branch 'main' of github.com:elastic/kibana into onweek/hot-upda…
afharo 8c1f684
Point APM to the gh repo until it is published
afharo 2d04781
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine 7b80030
clean kbn-journey & dataset-extractor
dmlemeshko 1543062
Merge branch 'main' of github.com:elastic/kibana into onweek/hot-upda…
afharo ecc1381
Merge branch 'main' of github.com:elastic/kibana into onweek/hot-upda…
afharo bdadfab
Hide behind feature-flag + add tests
afharo d668f76
Merge branch 'main' of github.com:elastic/kibana into onweek/hot-upda…
afharo 1d58c84
Fix tests
afharo 666a37c
Journey FTR harness to set the appropriate internal headers
afharo f768e8c
Remove accidental duplicated CLI option
afharo 97da881
[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…
kibanamachine 51f51c9
Merge branch 'main' into onweek/hot-update-of-apm-labels
afharo f3fbe27
Merge branch 'main' of github.com:elastic/kibana into onweek/hot-upda…
afharo 2bbc917
Add test to ping us when new options opt-in
afharo c6314f8
Merge branch 'main' into onweek/hot-update-of-apm-labels
afharo 19f114c
[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…
kibanamachine 730ebbc
Explicit `string` type to help TS inferrence
afharo 0fcc9e0
Add missing field to the telemetry/config response schema
afharo d5d42a8
Merge branch 'main' into onweek/hot-update-of-apm-labels
afharo 4e31733
Merge branch 'main' into onweek/hot-update-of-apm-labels
dmlemeshko acb62bb
Merge branch 'main' into onweek/hot-update-of-apm-labels
dmlemeshko File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
20 changes: 20 additions & 0 deletions
20
packages/core/apps/core-apps-server-internal/src/core_app_config.test.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
/* | ||
* 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 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
import { config, CoreAppConfig } from './core_app_config'; | ||
|
||
describe('CoreApp Config', () => { | ||
test('set correct defaults', () => { | ||
const configValue = new CoreAppConfig(config.schema.validate({})); | ||
expect(configValue).toMatchInlineSnapshot(` | ||
CoreAppConfig { | ||
"allowDynamicConfigOverrides": false, | ||
} | ||
`); | ||
}); | ||
}); |
48 changes: 48 additions & 0 deletions
48
packages/core/apps/core-apps-server-internal/src/core_app_config.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
/* | ||
* 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 and the Server Side Public License, v 1; you may not use this file except | ||
* in compliance with, at your election, the Elastic License 2.0 or the Server | ||
* Side Public License, v 1. | ||
*/ | ||
|
||
import { schema, type TypeOf } from '@kbn/config-schema'; | ||
import type { ServiceConfigDescriptor } from '@kbn/core-base-server-internal'; | ||
|
||
/** | ||
* Validation schema for Core App config. | ||
* @public | ||
*/ | ||
export const configSchema = schema.object({ | ||
allowDynamicConfigOverrides: schema.boolean({ defaultValue: false }), | ||
}); | ||
|
||
export type CoreAppConfigType = TypeOf<typeof configSchema>; | ||
|
||
export const CoreAppPath = 'coreApp'; | ||
|
||
export const config: ServiceConfigDescriptor<CoreAppConfigType> = { | ||
path: CoreAppPath, | ||
schema: configSchema, | ||
}; | ||
|
||
/** | ||
* Wrapper of config schema. | ||
* @internal | ||
*/ | ||
export class CoreAppConfig implements CoreAppConfigType { | ||
/** | ||
* @internal | ||
* When true, the HTTP API to dynamically extend the configuration is registered. | ||
* | ||
* @remarks | ||
* You should enable this at your own risk: Settings opted-in to being dynamically | ||
* configurable can be changed at any given point, potentially leading to unexpected behaviours. | ||
* This feature is mostly intended for testing purposes. | ||
*/ | ||
public readonly allowDynamicConfigOverrides: boolean; | ||
|
||
constructor(rawConfig: CoreAppConfig) { | ||
this.allowDynamicConfigOverrides = rawConfig.allowDynamicConfigOverrides; | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
(unrelated to this file, just writing as a comment to allow replying on the thread)
Overall, the technical implementation makes sense, and I don't have any strong problem regarding it.
My concerns / questions are more regarding the usages we're planning on doing of this feature.
We're basically implementing a per-node hot configuration update here. Which can be great. Now, these hot changes are not persisted. Any node rebooting will loose those hot updates, and reboot with the 'initial' configuration.
Which makes me wonder if we're planning any concrete production usage of the feature, or if the only planned usage is to update those labels in testing environments.
Because if that's the latest, then I can't avoid wondering if having that as a core API / endpoint makes sense compared to have it as a plugin that would just be in charge of updating the APM and EBT labels accordingly, without having to plug the feature so low level in core's config service.
OTOH, being able somehow to update dynamically some parts the config more easily than how it's currently done (I'm thinking, idk, about enabling debug logging via this endpoint, for instance) could be extremely powerful.
Curious to know what you think about this?
(also, if we're planning on merging, I think we're missing unit/functional/ftr tests on parts of the code that was modified in the PR)
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.
TL;DR, Yes to all! 😇
I initially implemented that. The main reason I ended up moving it to core was because the plugin couldn't inject the values to the initially rendered page, so APM and EBT initial events like
page-load
were wrongly labeled. Refer to Alternative options considered in the description.++ that'd be a great use case for this feature.
TBH, I have mixed feelings about this solution, especially for the caveats highlighted in What's not that good about this solution?. On the flip side, I think it's the best solution we can provide, given the current architecture.
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.
Ok, thanks for your explanation.
In that case, it could be a good idea to talk about it with the team in one of meeting to see what the others think about it?
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.
Yeah! In our last sync, I asked the team to have a look at this PR and the description to figure out if this is something we want to support, or if they can help me come up with alternatives.
I'll bring it up again in our next sync :)