-
Notifications
You must be signed in to change notification settings - Fork 518
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
[ACM] Agent remote configuration management - Main functionality #2095
Conversation
branch compiles and runs fine otherwise |
@simitt @graphaelli it would be great if you can have a look |
Took a look and the api and response make sense to me |
ba0cd64
to
3900229
Compare
aa792c2
to
fb27508
Compare
@graphaelli is there something missing/wrong here preventing merge, other than golint errors? |
I expected we were going to settle on the storage format before merging |
Worst case I would just have to remove this function: https://github.com/elastic/apm-server/pull/2095/files#diff-b6b7a562b4bbb8af1d07c8eaf1fda8b0R41, which is trivial (revert 5d3a467). Or am I missing something else? I'd really avoid PRs open for more than 1 month if possible |
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.
Great work @jalvz ! I left a couple of comments, most of them are nits.
agentcfg/fetch.go
Outdated
) | ||
|
||
var ( | ||
endpoint = "/api/apm/settings/cm/search" |
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: this could be a const
beater/config.go
Outdated
@@ -258,6 +259,7 @@ func defaultConfig(beatVersion string) *Config { | |||
filepath.Join("ingest", "pipeline", "definition.json")), | |||
}}, | |||
}, | |||
Mode: ModeProduction, | |||
Mode: ModeProduction, | |||
Kibana: nil, |
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.
Why are you setting a nil default?
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.
Because a default of common.Config()
would imply enabled=true
.
That means that after upgrading, users would be welcome with a new requirement (Kibana) or an error in the logs.
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. I'd be more explicit about it by either setting common.NewConfigFrom(`enabled: false`)
or adding a dedicated test to the config_test.go
checking that Kibana.Enabled() == false
. Otherwise it could easily happen that this gets removed in the future without anything failing.
@@ -183,6 +183,56 @@ apm-server: | |||
#ilm: | |||
#enabled: false | |||
|
|||
#kibana: |
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.
has there been a discussion around using the existing kibana settings instead of introducing a new one?
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 has been discussed.
This is the same setting, just in a different namespace.
The beats setting is under setup
which is not correct in our case; this is not for a setup command.
Also, should we use that, the server would refuse to start if Kibana is not reachable, which is not what we want, and not coherent eg. with Elasticsearch output.
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.
Gotcha 👍
beater/config.go
Outdated
@@ -258,6 +259,7 @@ func defaultConfig(beatVersion string) *Config { | |||
filepath.Join("ingest", "pipeline", "definition.json")), | |||
}}, | |||
}, | |||
Mode: ModeProduction, | |||
Mode: ModeProduction, | |||
Kibana: nil, |
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. I'd be more explicit about it by either setting common.NewConfigFrom(`enabled: false`)
or adding a dedicated test to the config_test.go
checking that Kibana.Enabled() == false
. Otherwise it could easily happen that this gets removed in the future without anything failing.
@@ -176,15 +176,14 @@ func (conn *Connection) Request(method, extraPath string, | |||
return resp.StatusCode, result, retError |
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.
Please remove this file before merging.
This introduces a new requirement (Kibana) and setting for it, in the apm-server namespace (disabled by default). It also adds an endpoint /v1/agent/configs for agents to poll configuration, requiring them to be authenticated (except RUM). Supersedes elastic#2095 More info in elastic/apm#4
Superseded by #2289 |
This introduces a new requirement (Kibana) and setting for it, in the apm-server namespace (disabled by default). It also adds an endpoint /v1/agent/configs for agents to poll configuration, requiring them to be authenticated (except RUM). Supersedes #2095 More info in elastic/apm#4
This introduces a new requirement (Kibana) and setting for it, in the apm-server namespace (disabled by default). It also adds an endpoint /v1/agent/configs for agents to poll configuration, requiring them to be authenticated (except RUM). Supersedes elastic#2095 More info in elastic/apm#4
This introduces a new requirement (Kibana) and setting for it, in the apm-server namespace (disabled by default). It also adds an endpoint /v1/agent/configs for agents to poll configuration, requiring them to be authenticated (except RUM). Supersedes #2095 More info in elastic/apm#4
Notes
ofc we can introduce an option to save the extra round trip.
Steps to test
Checkout [APM] Central configuration management kibana#36031, and start Elasticsearch and Kibana.
Create the CM index with defined mapping:
Alternatively, create a config directly in the UI by visiting
/app/apm#/settings/new
.You should have created some transactions first.
-E apm-server.kibana.enabled -E apm-server.kibana.path
properties set and send requests to the config endpoint:curl http://localhost:8200/config?service.name=apm-agent-js