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

[APM] Syncs agent config settings to APM Fleet policies #100744

Merged
merged 13 commits into from
Jun 8, 2021

Conversation

ogupte
Copy link
Contributor

@ogupte ogupte commented May 26, 2021

Closes #95501.

Synchronizes all agent configuration settings with existing APM fleet policies whenever any user creates/updates any agent configuration via API endpoint PUT /api/apm/settings/agent-configuration.

Can be tested with apm-integration-testing python3 ./scripts/compose.py start master --with-elastic-agent --apm-server-managed --all. Update an agent confuration in APM UI, then inspect the response from http://localhost:5601/api/fleet/package_policies to verify that the agent configs are updated in the apm package policy object.

@ogupte ogupte requested a review from a team as a code owner May 26, 2021 21:25
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label May 26, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

@ogupte ogupte added release_note:feature Makes this part of the condensed release notes v7.14.0 labels May 26, 2021
const configurations = await listConfigurations({ setup });
const agentConfigValue = configurations.map((configuration) => ({
service: configuration.service,
settings: configuration.settings,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
settings: configuration.settings,
config: configuration.settings,

Here's how things look in apm-server currentlly: https://github.com/elastic/apm-server/blob/91645bde2d96a79b943c90253d62da3861ab2997/apm-server.yml#L275-L297

  #---------------------------- APM Server - Agent Configuration ----------------------------
  # Directly specify agent configuration. If `agent_config` is set, agent
  # configuration under `kibana` will be ignored.
  # An agent's incoming configuration request will be matched to an
  # agent_config with the following precedence:
  # - service.name and service.environment match an agent_config
  # - service.name matches an agent_config, service.environment == ""
  # - service.environment matches an agent_config, service.name == ""
  # - an agent_config without a name or environment set
  # An empty result is is returned if no matching result is found.
  # agent_config:
  # - service.name: ten_percent
  #   service.environment: production
  #   config:
  #     capture_body: off
  #     capture_body: true
  #     log_level: info
  #     recording: true
  #     transaction_sample_rate: 0.1
  # - service.name: frontend
  #   agent.name: rum-js
  #   config:
  #     transaction_sample_rate: 0.1

We could alternatively change the apm-server implementation if you/anyone feels strongly about calling this "settings".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm ok changing it to conform with apm-server's implementation. thanks!

const agentConfigValue = configurations.map((configuration) => ({
service: configuration.service,
settings: configuration.settings,
}));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}));
etag: configuration.etag,
'agent.name': configuration.agent_name,
}));

By passing the etag value down, apm-server will (later) be able to use it when recording which config has been applied by agents. The UI can then match this up to the config blocks it knows about.

We also need agent.name for restricting config for untrusted agents (RUM and Jaeger)

…icy objects

- update package policy agent_configs to include etag, agent.name, and change settings -> config
@cauemarcondes
Copy link
Contributor

@ogupte could you give some instructions about how to test it, please?

@stuartnelson3
Copy link
Contributor

I was running this branch locally, and I have a couple questions. Forgive my ignorance if this has been answered elsewhere before (:grimacing:):

  1. I noticed my agent_config being rendered like this:
    image
    The config options are being rendered as strings, but they should be an int and a float. Is this expected?

  2. The agent_config and apm-server blocks are separate. Does agent_config get merged into apm-server when it's pushed? And, should it be nested under apm-server?
    image

setup,
});
logger.info(
`Saved latest agent settings to Fleet integration policy for APM.`
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you're logging something, maybe would be good to have a specific message when it is deleting?

Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

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

LGTM - Nice work @ogupte

@ogupte
Copy link
Contributor Author

ogupte commented May 27, 2021

I was running this branch locally, and I have a couple questions. Forgive my ignorance if this has been answered elsewhere before (😬):

1. I noticed my `agent_config` being rendered like this:
   ![image](https://user-images.githubusercontent.com/1398104/119853174-3e44a480-bf10-11eb-8424-3385917df860.png)
   The `config` options are being rendered as strings, but they should be an int and a float. Is this expected?

2. The `agent_config` and `apm-server` blocks are separate. Does `agent_config` get merged into `apm-server` when it's pushed? And, should it be nested under `apm-server`?
   ![image](https://user-images.githubusercontent.com/1398104/119853715-bad78300-bf10-11eb-8469-9ea26a048c5d.png)

@stuartnelson3, thanks for checking this out and testing it.

  1. config options are intentionally strings, sort of like how ENV variables are always strings. This was something we decided when we implemented agent configurations in the first place.
  2. While it's technically possible to combine Agent Configs and APM server configs into the same object, they configurations for different purposes, and it the APM server's responsibility to deliver agent configurations to the appropriate agent. If folks working on apm-server preferred them combined, i'll be happy to accommodate. @axw

@axw
Copy link
Member

axw commented May 28, 2021

config options are intentionally strings, sort of like how ENV variables are always strings. This was something we decided when we implemented agent configurations in the first place.

👍 sorry @stuartnelson3, I missed that we had numbers in our config in the apm-server.yml example. I just checked the code and it's a map[string]string so I think we just need to update the example.

While it's technically possible to combine Agent Configs and APM server configs into the same object, they configurations for different purposes, and it the APM server's responsibility to deliver agent configurations to the appropriate agent. If folks working on apm-server preferred them combined, i'll be happy to accommodate. @axw

I would prefer that everything specific to APM is nested under the apm-server key, so apm-server.agent_configs in this case.

@stuartnelson3
Copy link
Contributor

stuartnelson3 commented Jun 1, 2021

this ended up being correctly sent to apm-server, but I suppose it'd be nice to have it rendered within the apm-server config block:

diff --git a/x-pack/plugins/apm/server/lib/fleet/register_fleet_policy_callbacks.ts b/x-pack/plugins/apm/server/lib/fleet/register_fleet_policy_callbacks.ts
index bfc9d360..8ff34d96 100644
--- a/x-pack/plugins/apm/server/lib/fleet/register_fleet_policy_callbacks.ts
+++ b/x-pack/plugins/apm/server/lib/fleet/register_fleet_policy_callbacks.ts
@@ -106,7 +106,7 @@ export function getPackagePolicyWithAgentConfigurations(
       {
         ...firstInput,
         config: {
-          agent_config: {
+          'apm-server.agent_config': {
             value: agentConfigurations.map((configuration) => ({
               service: configuration.service,
               config: configuration.settings,

in the UI:
image

EDIT: I'm not sure what's going on with the rendering of span_frames_min_duration, though

@ogupte
Copy link
Contributor Author

ogupte commented Jun 1, 2021

@elasticmachine merge upstream

@ogupte
Copy link
Contributor Author

ogupte commented Jun 2, 2021

this ended up being correctly sent to apm-server, but I suppose it'd be nice to have it rendered within the apm-server config block:
...

@stuartnelson3 can you confirm that this works: https://github.com/elastic/kibana/pull/100744/files#diff-ab7e62a4c711e2133e13004dec4ecfedd8bb1bf46da87eed88f61bd743b2ffd4R118-R128

@ogupte
Copy link
Contributor Author

ogupte commented Jun 2, 2021

@elasticmachine merge upstream

@stuartnelson3
Copy link
Contributor

stuartnelson3 commented Jun 2, 2021

I started from a fresh ES + new build of kibana off this branch, and when I tried to add the APM server integration I got this error:

image

cause: SchemaTypeError: definition for this key is missing

looks like static typing is at it again

…-server']

in order to pass validation checks
@ogupte
Copy link
Contributor Author

ogupte commented Jun 2, 2021

@stuartnelson3 in order to fix the validation error I had to define a property value for config['apm-server'] within the package policy. Overall the structure looks like this: packagePolicy.inputs[0].config['apm-server'].value.agent_config = Array<{service, etrag, config: {...}}>. I also see there's a compiled_input object in the input, that contains apm-server (packagePolicy.inputs[0].compiled_input['apm-server'] = { api_key, secret_token, host, ... }), but i assume we want to continue to use config. Can you confirm if this schema works for apm-server?

Screen Shot 2021-06-02 at 4 40 24 PM

- parallelizes operations for improved performance
@stuartnelson3
Copy link
Contributor

config is correct, cool. I pulled in the changes and no longer get the validation error when creating the integration, but after adding an agent config, I'm not seeing it rendered in the UI:
image

@ogupte
Copy link
Contributor Author

ogupte commented Jun 7, 2021

config is correct, cool. I pulled in the changes and no longer get the validation error when creating the integration, but after adding an agent config, I'm not seeing it rendered in the UI:

@stuartnelson3 It looks like the fleet kibana code which generates that yaml spreads in the contents of config followed by the contents of compiled_input, and since they both have properties named apm-server the latter overwrites the former: https://github.com/elastic/kibana/blob/master/x-pack/plugins/fleet/common/services/package_policies_to_agent_inputs.ts#L34-L38

is apm-server consuming these agent configs with the raw policy object, or only through this output yaml?

@stuartnelson3
Copy link
Contributor

@ogupte I'm not sure, I've been doing manual testing by copying+pasting the output yaml. @axw had surmised this to possibly be the case -- maybe he also knows if the raw policy object is being pushed or not?

Comment on lines +117 to +118
config: {
[APM_SERVER]: {
Copy link
Contributor

Choose a reason for hiding this comment

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

@ogupte Don't you need to add the others config here to avoid overwriting the entire config object?

Suggested change
config: {
[APM_SERVER]: {
config: {
...firstInput.config,
[APM_SERVER]: {

@axw
Copy link
Member

axw commented Jun 8, 2021

is apm-server consuming these agent configs with the raw policy object, or only through this output yaml?

@ogupte apm-server will receive the combined output yaml. Maybe flatten the config string, so it's added like apm-server.agent_config rather than as a nested object structure.

@ogupte
Copy link
Contributor Author

ogupte commented Jun 8, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@ogupte ogupte merged commit ff0349d into elastic:master Jun 8, 2021
ogupte added a commit to ogupte/kibana that referenced this pull request Jun 8, 2021
* [APM] Syncs agent config settings to APM Fleet policies (elastic#95501)

* fixes eslint issues

* fixes malformed line comment

* - consolidated logic that applies agent configurations to package policy objects
- update package policy agent_configs to include etag, agent.name, and change settings -> config

* Synchronizes agent configs whenever configuration is deleted.

* PR feedback

* nest agent_config within `apm-server` in the package policy input

* nests agent_config under the requried 'value' property of config['apm-server']
in order to pass validation checks

* - externalizes getApmPackagePolicies for reusability
- parallelizes operations for improved performance

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
ogupte added a commit that referenced this pull request Jun 9, 2021
…01685)

* [APM] Syncs agent config settings to APM Fleet policies (#95501)

* fixes eslint issues

* fixes malformed line comment

* - consolidated logic that applies agent configurations to package policy objects
- update package policy agent_configs to include etag, agent.name, and change settings -> config

* Synchronizes agent configs whenever configuration is deleted.

* PR feedback

* nest agent_config within `apm-server` in the package policy input

* nests agent_config under the requried 'value' property of config['apm-server']
in order to pass validation checks

* - externalizes getApmPackagePolicies for reusability
- parallelizes operations for improved performance

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 9, 2021
* master: (54 commits)
  Implement "select all" rules feature (elastic#100554)
  [ML] Remove script fields from the Anomaly detection alerting rule executor  (elastic#101607)
  [Security solutions][Endpoint] Update event filtering texts (elastic#101563)
  [Enterprise Search] Mocks/tests tech debt - avoid hungry mocking (elastic#101107)
  [FTR] Updates esArchive paths
  [FTR] Updates esArchive paths
  [Security Solution][Detection Engine] Adds runtime field tests (elastic#101664)
  Added APM PHP agent to the list of agent names (elastic#101062)
  [CI] Restore old version_info behavior when .git directory is present (elastic#101642)
  [Fleet] Add fleet server telemetry (elastic#101400)
  [APM] Syncs agent config settings to APM Fleet policies (elastic#100744)
  [esArchiver] drop support for --dir, use repo-relative paths instead (elastic#101345)
  Revert "[xpack/test] restore incremental: false in ts project"
  [Security Solution] Remove Host Isolation feature flag (elastic#101655)
  [xpack/test] restore incremental: false in ts project
  [DOCS] Adds link to video landing page (elastic#101413)
  [ML] Move Index Data Visualizer into separate plugin (Part 1) (elastic#100922)
  Improve security plugin return types (elastic#101492)
  [ts] migrate `x-pack/test` to composite ts project (elastic#101441)
  [App Search] Updated Search UI to new URL (elastic#101320)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:feature Makes this part of the condensed release notes Team:APM All issues that need APM UI Team support v7.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[APM] Inject agent config directly into APM Fleet policies
6 participants