Following #4 , agents need to be able to poll apm-server for configuration changes received upstream from Kibana, apply them, and log the result with status (success | failure), failure cause (if any), timestamp, setting name and value.
We will start providing support for TRANSACTION_SAMPLE_RATE.
At minimum, agents need to agree on:
/config/v1/agents endpoint, for agents to GET with a service.name URL query parameter (required), and service.environment (optional)Agents might send a request with a If-None-Match header, to which Server will respond with a 304 - not modified response; or with 200, a response body with the configuration, and an Etag header.
Example
curl -v -H "If-None-Match:1" "http://localhost:8200/config/v1/agents?service.environment=prod&service.name=opbeans"
* Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8200 (#0)
> GET /config?service.environment=prod&service.name=opbeans HTTP/1.1
> Host: localhost:8200
> User-Agent: curl/7.61.0
> Accept: */*
> If-None-Match:1
>
< HTTP/1.1 200 OK
< Cache-Control: max-age=0
< Content-Type: application/json
< Etag: 2
< Date: Thu, 11 Apr 2019 14:08:32 GMT
< Content-Length: 27
<
{
"transaction_sample_rate": 0.7
}
Configuration settings are taken from the environment variable names, without the ELASTIC_APM_ prefix, and lower case.
As per comment https://github.com/elastic/apm/issues/76#issuecomment-487470018, the Server will accept query parameters both in the URL and in the body of a POST request.
If different values for the same attribute are provided as POST and GET, request will be 400-rejected; different attributes will be joint.
Other notes that slipped in the initial description:
As pointed out in https://github.com/elastic/apm/issues/76#issuecomment-484789567, agents should also align the error handling behaviour. For instance, if a config update can't be applied, should fallback to the last good value, to the agents default value, or to the value that the process started with?
Regarding service.environment, if none is passed in the query, only config updates without service environment will match. Likewise, if one is passed, only config updates with that value will match (and not config updates without value). In other words, a missing service environment is treated like any other (with a value of "" if you want to see it that way).
Cache-Control: max-age header in every successful response. For failing agent requests the header will be set with a max-age: 300 (5 mins) since querying again after 30s doesn't make sense. Decision was made to set to 5 mins instead of not setting or setting to 0 so agents don't need to put their own logic and can differentiate between server not supporting remote config and failures. More details on this in https://github.com/elastic/apm-server/issues/2220.7.3403 if ACM not enabled in APM Server (apm-server.kibana.enabled: false) (https://github.com/elastic/apm-server/pull/2386)Etag header in double quotes and expect the same for the If-None-Match header (https://github.com/elastic/apm-server/pull/2407)json if supported and return minimal error response body if no security token is used for communicating with the APM Server (https://github.com/elastic/apm-server/pull/2421)200 with empty body instead of 404 if configuration has not been found (https://github.com/elastic/apm-server/pull/2458)Etag from _source.settings and return flattened key:value pairs in case nested config options are stored (https://github.com/elastic/apm-server/pull/2441).@elastic/apm-agent-devs please link your implementation issues
Let me know if you have any questions.
Are there plans to create configurations for a more fine-grained subset of agents, based on more than service_name and service_environment possible even via global labels? If so, it might make sense to send the metadata along with the request.
A minor comment: the name for sampling rate option is "TRANSACTION_SAMPLE_RATE" (https://docs.google.com/spreadsheets/d/1JJjZotapacA3FkHc2sv_0wiChILi3uKnkwLTjtBmxwU/) so I would suggest to use "transaction_sample_rate" in response JSON for consistency.
Should agents be aligned on other aspects of error handling and not just error reporting? For example, if some of the options fail to pass validation should the agent reject the whole configuration or should the agent use the valid options and find some replacements for the invalid ones (use default values, etc.)
name for sampling rate option is "TRANSACTION_SAMPLE_RATE"
thanks @SergeyKleyman , updated with a note.
Should agents be aligned on other aspects of error handling
Yes, good point. I did highlight error reporting because it might have an impact in Kibana (eg. someone could create dashboards based on it), so I think it is important for the feature as a whole.
You should discuss error handling and align to the the extent of what is reasonable, but it is essentially up to you.
Are there plans to create configurations for a more fine-grained subset of agents
It was discussed briefly. We can ado that, yes, but it doesn't have the highest priority atm. Any ideas here ofc are welcome.
It was discussed briefly. We can ado that, yes, but it doesn't have the highest priority atm. Any ideas here ofc are welcome.
Sure, we don't have to start with that right away. My point is that even if we want to add support for it later, we should already start sending the metadata instead of query parameters now.
Sending metadata instead of query parameters does seem like it would make things more extensible. A newer server could start taking advantage of metadata agents already send, without any upgrade of agents. Is this difficult for anyone to implement?
At least for the RUM agent being able to get the configuration without sending a body is preferable. Furthermore I think the extensibility provided by sending metadata is not very useful when the available configuration options is limited to a small number as is the case for the RUM agent.
We can accept both, if that makes happy everyone.
@jalvz that sounds fine to me, if it's not too onerous on the server.
Could we just say that you can specify any metadata as a query parameter, using the same JSON path? e.g. where a backend agent might use
GET /config
{
"service": {
"name": "foo",
"environment": "bar"
}
}
the RUM agent would use
GET /config?service.name=foo&service.environment=bar
i.e. with dots instead of _, so we can directly map the query params to a nested object in the metadata. Then, for completeness, we would state that query parameters would be overlaid on the request body (if any).
@elastic/apm-agent-devs can I get you to comment on:
top 2-5 settings that should follow sampling rate
Some interesting ones are LOG_LEVEL, METRICS_INTERVAL, SPAN_FRAMES_MIN_DURATION, TRANSACTION_MAX_SPANS, SERVER_TIMEOUT, INSTRUMENT... what would you like to see next?
I'd like to see that users can enter any configuration options via text input.
what do you win with that?
(agents have to implement dynamic loading on their side, so we still need that list regardless)
That lets users define settings which are agent specific or were added recently so that the config UI does not know about them yet.
(agents have to implement dynamic loading on their side, so we still need that list regardless)
Not sure what we are actually talking about here. Do you mean 2-5 settings the UI should support next or the settings that the agents should support when reading the /config endpoint? I think the agents should generically recognize all the options they support from the contents from the /config endpoint. Therefore, we don't need to specify a comprehensive list of the options at the API level.
Besides that, support for the ACTIVE flag would be important as well so that agents can be disabled at runtime.
That lets users define settings which are agent specific
UI can support settings specific for each agent any case, no need blank text input just for that.
or were added recently so that the config UI does not know about them yet
I'll argue that with existing release cycles this is not too bad (plus: with a settings-aware UI we give users some incentive to upgrade!)
Not sure what we are actually talking about here
How the user will know what settings are reloadable by each agent? That is what I meant above, unless you all implement dynamic reloading for all settings for the next version, users need to know what works and what not.
ACTIVE flag would be important
This is the one named INSTRUMENT, no?
Other counter arguments:
Giving feedback to users is not trivial (and left out in first phase), so it is important that UI can do validation upfront, which is much harder that way (eg: validate numeric ranges for specific settings).
What to do if user enters a typo? let it go? Validate against a mapping of known settings? do we need a DYM for that?
Generally a text input is more prone to mistakes, requires more documentation detailing the expected behaviour and is harder to use as users have no guidance whatsoever
How the user will know what settings are reloadable by each agent?
I think the agent configuration options documentation should state for each config whether it's reloadable. The Java agent already does that: https://www.elastic.co/guide/en/apm/agent/java/current/configuration.html (dynamic true/false). So each setting with dynamic=true should work out of the box.
I do get the point that validation is easier when restricting the options. What do you think about validating the values for known options but still allowing to set options the UI does not yet know about via a free-text input? Users already have the possibility to set options as free text in the configuration files and environment variables so the problem of misspelled options and invalid values is not entirely new. The impact may be more severe and harder to debug with central CM, however
Even if we choose to restrict the options via the UI, I wouldn't restrict it on the API level.
Question: should we define a common format which the agents provide and the UI uses in order to suggest certain settings, along with type and validation metadata?
ACTIVE flag would be important
This is the one named INSTRUMENT, no?
ACTIVE and INSTRUMENT are two different things. I suspect that most agents can't make INSTRUMENT reloadable, in contrast to ACTIVE.
Another question: Should agents query the /config endpoint on startup so that it's possible to configure non-reloadable options? Probably not, right? At least not for the first iteration. Because we would have to block the startup and we need the metadata to call /config but the metadata also contains values from the configuration (like service_name).
I wouldn't restrict it on the API level
No, we're not doing that
should we define a common format which the agents provide
I think everything that helps UI to do validation is a great idea :+1:
Should agents query the /config endpoint on startup
Agreed, better not for now
I'd like to see that users can enter any configuration options via text input.
This will be very useful for supportability.
I have a small suggestion - maybe we can hide this section of UI under "Advanced" sub-view or something like that?
If we decide to go with configuration options via text input another point we need to consider is what is the expected outcome when the same option is provided twice
(with different values) - once via structured UI and once via text input?
Alright, lets keep those arguments for the next iteration.
I added an update to the initial description, please everyone go trough it and let me know if you have anything against.
@SergeyKleyman that got me thinking: What does the APM Server return when multiple configurations match the current agent? Is it guaranteed that only one can ever match? If so, how? If not, how are configurations merged?
Also, how should the configurations from APM Server overrule the other configuration sources? The current hierarchy for the Java agent is (from highest to lowest precedence):
As per comment #76 (comment), the Server will accept query parameters both in the URL and in the body of a POST request.
The contents of the post would be of type application/json and not application/x-www-form-urlencoded, right?
Regarding what we can configure in the future, the only config options that the agents currently all agree on are (with "agree on" I mean where we agree both on the config option name and how its value should be interpreted):
API_REQUEST_SIZEAPI_REQUEST_TIMECAPTURE_BODYCAPTURE_HEADERSSECRET_TOKEN (not a candidate for remote configuration unless we want the /config endpoint to be accessible without a secret token)SERVICE_NAME (obviously not a candidate for remote configuration)SERVICE_VERSION (obviously not a candidate for remote configuration)TRANSACTION_MAX_SPANSTRANSACTION_SAMPLE_RATEVERIFY_SERVER_CERT (not really a candidate for remote configuration I think)Unless the UI allows a user to send some config only to Node.js agent while some other config only to Java agents, we have to currently limit our selves to these config options.
There are a few other options that are not implemented by all agents, but where the subset of agents who have implemented them do all agree. Those might be candidates as well.
Regarding the discussion about being able to remotely set the ACTIVE flag: This will be a one-time thing.
Once ACTIVE is set to false, the agent will be disabled and therefore not poll /config again and hence never see if we set it back to true. So I think this belongs in the same category as SECRET_TOKEN and VERIFY_SERVER_CERT, where you can kind of only set it once, but then never again and hence isn't really that use-full in a remote config scenario.
Should we have a separate issue to align on meaning of ACTIVE? Judging by https://discuss.elastic.co/t/unable-to-disable-apm-via-environment-variable/180558 some users expect to have a configuration setting to completely disable the agent - even communication to APM Server should be disabled.
I think that turning the agent on and off remotely is a great feature to have. Therefore, it's worth discussing whether agents should poll the /config endpoint even when active is false. That should, however, be about the only thing the agent does. For example, sending metrics should also be disabled. If there are connection errors, we should probably not bail out every time.
I think it's important to have a mode where the agent is completely inactive - which includes not trying to poll /config. But I agree it will be nice to be able to turn the agent on and off remotely as well.
So maybe we can introduce a different state where the agent is inactive, while still polling for config?
Even if active is false, there will still be some agent code running in the application because we don't and sometimes can't un-instrument the application. The agent merely goes into a noop-mode. Maybe we need a config for the remote configuration polling interval which disables when set to 0 or negative.
In the case of the Node.js agent, if it's started with active: false, there's no way to later set active to true. It's technically impossible the way active: false is implemented in the Node.js agent today. If it's started with active: true, we can make a feature where it later goes into no-op mode. It's not something we have today, but it would be fairly trivial to implement.
Maybe we should introduce several states?
active: false in the Node.js agent today)/config like normal, but all other network activity is disabled./config like normal, but all other network activity is disabled.active: true in the Node.js agent today)The "No-op" and "Mock" state are almost identical and could be merged into one if we felt like it. But there might be certain value in being able to differentiate 🤔
For the Ruby agent currently:
active: false does this.disable_send: true does this.@mikker what's the idea behind "disable_send"? Is this used for debugging the agent? Say, disable sending and then observe behaviour through logging?
In the case of the Node.js agent, if it's started with active: false, there's no way to later set active to true. It's technically impossible the way active: false is implemented in the Node.js agent today. If it's started with active: true, we can make a feature where it later goes into no-op mode. It's not something we have today, but it would be fairly trivial to implement.
@watson I just looked at the Node.js agent code, and I think I see what you mean about it being impossible the way things are implemented today. If I understand correctly, active: false disables require-in-the-middle, and that's not something that can be reversed.
Does it have to remain that way? Could we have active: false install the hooks, but immediately go into no-op mode?
I just looked at the Node.js agent code, and I think I see what you mean about it being impossible the way things are implemented today. If I understand correctly,
active: falsedisables require-in-the-middle, and that's not something that can be reversed.
Correct. The first time a given module is required we get a chance to hook into it. If we don't do it then, we "can't change our mind" and do it later.
Does it have to remain that way? Could we have
active: falseinstall the hooks, but immediately go into no-op mode?
I'm open to discussing if we can change this behavior. But this doesn't just apply to active, but also to disableInstrumentations. One of the reasons why we have these options is in case the agent somehow misbehaves and the user wants to disable the hook. If the agent misbehaves, it's most likely because of the way we patch a specific module and giving the user the ability to disable the agent without removing the agent entirely and redeploying the code is a good feature to have.
@axw what's the idea behind "disable_send"?
It's the _turn it off_ setting I recommend for test or development for example. Goes through the whole agent up to the point of opening a connection to APM Server. Lets you keep all you agent related code and config and know if things are awry before going to production. I think a few agents have an option named so.
I've created https://github.com/elastic/apm/issues/92, let's continue discussion of active/disabledInstrumentations there.
Testing the agent-server API with @felixbarny it came up the question of whether the metadata should have the metadata key at the top, or just its contents (service, system, etc).
One argument for the later is that it will match the structure of the query URL params (simply service.name, service.environment).
Not a very interesting question, but everyone OK with that? Any other thoughts?
Another question is what the type of the configuration values should be. Currently, the /config endpoint would return the value in the actual type. For example 0.7 for sampling_rate and {"foo": "bar", "baz": "qux"} for global_labels. Currently, there is no explicit JSON schema which defines the type for each value but the Elasticsearch mapping does have explicit mappings. That is different from other configuration sources, like environment variables where global_labels would be specified as foo=bar, baz=qux.
Maybe it's just something which is specific for the Java agent but this is going to be quite problematic and would probably require a major re-architecture of the configuration source mechanism. The Java agent defines an interface for configuration sources which requires to return a string value for a given string key. Because the /config endpoint does not return strings but typed values, we can't easily make a configuration source for that like we already did for property files, system properties and environment variables where the value is always a string, which needs to be parsed by the agent.
All configuration management systems I'm aware of, for example Spring Cloud Config, also just have the concept of string-typed key/value pairs.
Before diving into why this is difficult for the Java agent and what the pros/cons of string vs typed values are, I want to gauge from other agent devs (specifically but not only limited to those which write an agent for strictly typed languages) if that's going to be a problem for them.
It's not going to be an issue for .NET agent because even for environment variables we mapped configuration to a DTO and the rest of the agent only sees that DTO and it is not aware if the source was string to string map or something else.
Regarding your issue, I wonder: can you convert JSON to a flat string to string map before passing it on to existing configuration processing code?
Not sure if I fully understood the original questions, but just as @SergeyKleyman mentions for the .NET agent, because that we have to accept input from environment variables (where everything is a string value), we in the Node.js agent also had to expect all config options potentially to be strings and do the format conversion in the source code:
@felixbarny It wouldn't be a problem in the Go agent, but then I don't have an abstracted config provider. I just have methods on the tracer for updating config, and those accept typed values. The remote config polling code would call those, after having parsed the JSON.
As Sergey suggests, you could stringify the values agent-side.
My first reaction was that it seems a little wasteful to me for the server to drop the types. However, we should consider whether this complicates upgrades.
If we provide the ability to distribute arbitrary config, then that would be used when the UI is unaware of any config schema. The UI would not parse values for unknown config attributes, and so they would always end up at the agent as strings. Then if we later add a schema for these config attributes to UI, the UI would start parsing/validating and sending them as typed values. The agent then needs to deal with two types.
If I understood @felixbarny correctly the issue is not just strings vs other scalar type (numbers and booleans) but also flat map vs hierarchical JSON which I assume would require more sophisticated code to flatten “properly” since in some cases flattening would be done using keys (
a.b.c...) while in other cases it would be done by converting container to string and placing it into value (as in example of global_labels Felix gave). I agree that your suggestion @axw will simplify the agent side but it would mean that when the backend wants to start supporting a config setting (with validation, etc.) the backend will also have to know how to “flatten” it for agents. Although in the vast majority of cases those will be scalar values so just converting to string will do the trick.
The UI would not parse values for unknown config attributes, and so they would always end up at the agent as strings. Then if we later add a schema for these config attributes to UI, the UI would start parsing/validating and sending them as typed values. The agent then needs to deal with two types.
Exactly. This is also one of my biggest concerns. That would basically disallow to add a free-text configuration input at some later point. It's not only the agent which would have to deal with multiple types but also Elasticsearch which would make things really messy. For example, if it's initially typed as string due to a plain text input but at a later point, the APM Server changes the schema to a number.
Flattening to a string is easy for scalar values but as soon as it comes to lists or even complex objects it will become more challenging. For the Java agent, we'd probably also have to create a custom DTO which is aware of all the types and use the appropriate value converter to convert back to a string just to fulfill the ConfigurationSource contract. It's possible but complex - not only initially but we also have to maintain two sets of types going forward: the DTO and the configuration descriptors (example)
We would also lose the ability to make backwards compatible type changes as we did with server_url which used to be a scalar string but can now be a list of strings on some agents.
For the Java agent, we'd probably also have to create a custom DTO which is aware of all the types and use the appropriate value converter to convert back to a string just to fulfill the
ConfigurationSourcecontract. It's possible but complex - not only initially but we also have to maintain two sets of types going forward: the DTO and the configuration descriptors (example)
@felixbarny The approach we are considering for .NET is to put schema description (what you call configuration descriptors) as attributes (annotations in Java parlance) on DTO properties which should keep everything in one place.
Offline conclusion: other agents are fine with either strings or concrete types, so the Server will send strings only. Added an update to the original description.
Server support has landed: https://github.com/elastic/apm-server/pull/2289
Endpoint has been renamed to /v1/agent/configs, I have updated the description here (cc @felixbarny)
Shouldn't it be /agent/v1/configs? (or something like that)
So far we had the following API endpoints:
intake/v2/events
intake/v2/rum/events
assets/v1/sourcemaps
Here are my thoughts around this structure: intake and assets each group a dedicated _API_, where events and sourcemaps are _resources_ and rum is a _namespace_.
So currently we have the format API/version/{namespace}/resource.
Adding the configs endpoint as v1/agent/configs doesn't define a dedicated API type. It puts agent as _namespace_, while configs is the _resource_. If we ever also want to expose the server's config via an endpoint we could simply add v1/configs without a namespace (same as default backend events don't have a namespace on the intake API).
The other option would be to create a dedicated _agent_ API of the format agent/v1/config. If we ever introduce a server config endpoint this would need to go to a different API. IMO this would also be fine, just have slightly different semantics and probably would make sense to introduce if we plan to have multiple different agent endpoints in the server.
There was not a lot of discussions around the endpoint versioning and naming.
@watson why would you prefer/expect the endpoint as you described?
Had a discussion with @simitt and @jalvz on Zoom about this. We decided we liked /config/v1/agents the most. If anyone is against that, please speak up. Otherwise @jalvz will make a PR with this change tomorrow 😃
cc @elastic/apm-agent-devs @roncohen
To get a working dev environment where I could test the agent against the stack, I did the following (with the help of @simitt) - hopefully this can be helpful for others who start adding this feature:
master). From within the apm-integration-testing repo root, run the following:
./scripts/compose.py start master --no-apm-server
master of the apm-server repo and build it (make + make update). I need to prefix the make commands with CC=gcc as gcc isn't my default compiler.apm-server.yml file setting kibana.enabled: true (uncomment the kibana: line + inside that group, uncomment the enabled: line, changing its default value to true).
./apm-server -c apm-server.yml -e -d "*"
Open the Kibana dev console and run the following commands to add test data (or be creative) [source]:
PUT .apm-cm/_doc/1
{
"service": {
"name": "opbeans-lisp",
"environment": "lambda"
},
"@timestamp" : "2019-03-01T12:12:12",
"settings": {
"transaction_sample_rate": 0.1
}
}
PUT .apm-cm/_doc/2
{
"service": {
"name": "opbeans-fortran",
"environment": "rocket-launchpad"
},
"@timestamp" : "2019-04-01T12:12:12",
"settings": {
"transaction_sample_rate": 0.4
}
}
PUT .apm-cm/_doc/3
{
"service": {
"name": "opbeans-fortran",
"environment": "rocket-launchpad"
},
"@timestamp" : "2019-07-01T12:12:12",
"settings": {
"transaction_sample_rate": 0.7
}
}
PUT .apm-cm/_doc/4
{
"service": {
"name": "opbeans-fortran",
"environment": "testbed"
},
"@timestamp" : "2019-10-01T12:12:12",
"settings": {
"transaction_sample_rate": 1.0
}
}
PUT .apm-cm/_doc/5
{
"service": {
"name": "opbeans-fortran"
},
"@timestamp" : "2019-12-01T12:12:12",
"settings": {
"transaction_sample_rate": 0.12
}
}
Run a curl command against the APM Server to test everything is working (I used opbeans-fortran because this name was added in the test data above):
$ curl -i http://localhost:8200/config/v1/agents\?service.name\=opbeans-fortran
HTTP/1.1 200 OK
Cache-Control: max-age=30, must-revalidate
Content-Type: application/json
Etag: 5
Date: Thu, 04 Jul 2019 13:50:11 GMT
Content-Length: 30
{
"sampling_rate": "0.12"
}
Try to add the If-None-Match header with the value of the Etag to see that this also works:
$ curl -i -H "If-None-Match:5" http://localhost:8200/config/v1/agents\?service.name\=opbeans-fortran
HTTP/1.1 304 Not Modified
Cache-Control: max-age=30, must-revalidate
Etag: 5
Date: Thu, 04 Jul 2019 13:54:38 GMT
Update 7/7: I updated the 5 document examples above, to use the agreed upon transaction_sample_rate name instead of sampling_rate.
You can still do that if you wish, but now that the Kibana side is completed you can fire up some opbeans and use the actual Kibana APM UI to create the settings instead of PUTing docs into elasticsearch. That has the added benefit of testing how the UI feels :)
For the server you can also run it like ./apm-server -e -E apm-server.kibana.enabled=true instead of editing the yml file.
This is not clear in the issue description, but according to https://github.com/elastic/apm-server/issues/2220#issuecomment-506580439, the agent should fall back to a 5-minute polling interval in case no Cache-Control header is returned, or in case of a connection failure.
Is this correctly understood?
We agreed on the server sending 5 mins for retrying for failed requests, so yes, I'd assume the agents use this same value as a fallback in case the server cannot respond with this header.
I assume the configuration received from the APM Server should overrule configuration set by any other means - i.e. it should take precedence over environment variables, any local config files, and inline config?
We agreed offline that nothing related to ACM would be configurable on the agent, so there shouldn't be any polling interval set via ENV variables, config files etc.
Besides my question above, there are a few things we need to agree upon. Here's how we've implemented those in the Node.js agent.
The Node.js agent has named the config option to enable this feature remoteConfig and it's off by default. In the documentation, it specifies that APM Server 7.3 or later is required (PR: elastic/apm-agent-nodejs#1197)
If the Node.js agent receives any config option it doesn't explicitly know about (currently anything other than transaction_sample_rate), it will log the following at the "warning" level:
Remote config failure. Unsupported config names: unknown_option, other_unknown_option
If it receives a config option that it knows about, but a value that is invalid (e.g. if the sample rate is above 1), it logs one line per invalid value also at the "warning" level, e.g:
Remote config failure. Invalid value for transactionSampleRate: 1.2
If it successfully manages to update a config option, it will log the following at the "info" level:
Remote config success. Updating transactionSampleRate: 0.1
Notice how the two latter log statements use the Node.js styled versions of the config option (camel case instead of snake case). This is to make our code simpler as we convert the spelling received from the APM Server as early as possible to be able to do all the normalization and validation with our existing logic.
@watson yes, I think config via Kibana should have highest precedence.
The Node.js agent has named the config option to enable this feature remoteConfig and it's off by default. In the documentation, it specifies that APM Server 7.3 or later is required
Why is it disabled by default? Because of errors when using an old server? To minimise user effort, I'd prefer if we had it enabled by default, and log a message at debug level if the server returns 404. Is there some downside to that approach?
I hadn't planned on adding any config to disable this feature, apart (later) the enabled flag we started discussing in https://github.com/elastic/apm/issues/92.
The log messages you use look good. I would probably add the reason for why the config value is invalid.
Why is it disabled by default? Because of errors when using an old server? To minimise user effort, I'd prefer if we had it enabled by default, and log a message at debug level if the server returns 404. Is there some downside to that approach?
Yes, mainly in case people are using an old server or a server where this isn't enabled (it's also disabled by default in the APM Server currently). This is a boarder-line breaking change depending on how you look at it. And releasing it as disabled by default, means we don't have to consider the downsides as it's opt-in. If we suddenly start posting an error-level log message every 5 minutes, some users might be annoyed.
The reason why we're logging at an error level is that we consider all failed communication with the APM Server an error. We can keep that logic in place if we make it opt-in whereas if it was enabled by default, we'd have to handle these types of connection errors differently, which would require some bigger changes due to the internal structure of the Node.js agent.
I hadn't planned on adding any config to disable this feature, apart (later) the enabled flag we started discussing in #92.
To be able to use the agent with a pre 7.3 APM Server, or where this feature is disabled in the APM Server, I think it's a requirement that we have a config option to turn this feature off.
Update: It would also be annoying to the people operating the APM Server as the APM Server would keep logging 500 errors in its own log every time an agent sent a config request if this feature is disabled in the APM Server.
The log messages you use look good. I would probably add the reason for why the config value is invalid.
Currently, our validator just gives a boolean result. But I guess we could refactor it to give an error message instead. But I didn't consider this super important as the rules are all documented. Is this important you think?
If the Node.js agent receives any config option it doesn't explicitly know about (currently anything other than transaction_sample_rate), it will log the following at the "warning" level:
I would advise against that. I think agents should implement it more generically, just like with environment variables. The reasoning is that users then don't have to update the agent when Kibana supports more than this configuration option. Plus - the same code as for reading config from env can be reused. I don't see a good reason for locking it down to just the sample rate.
I'm also in favor of enabling it by default. We can detect that the APM Server does not support remote config and log at info-level once.
I think we need some kind of specification or pseudo-code how agents should handle central config, otherwise, we'll end up with different behavior for each agent. I would prefer that to be in the form of a PR to agent-development.md (or a new file in the same folder). That should, in the future, the single source of truth for the specification of an agent. If there are changes, amendments or new features, that should be reflected as a PR.
Update to https://github.com/elastic/apm/issues/76#issuecomment-508491565: With https://github.com/elastic/apm-integration-testing/pull/523 ACM is enabled by default now in the integration testing, so no need for building APM Server yourself any more. You can disable it by passing in --apm-server-acm-disable flag.
@felixbarny and I just had chat about this to avoid too many back and forths. We, unfortunately, didn't reach any solid conclusions, but here's the gist (Felix, please chime in if I misrepresent your opinion):
We read the issue a bit differently. I read it as the agent should only support the currently agreed upon config option (transaction_sample_rate) and therefore log a warning in case the agent saw any other config option. Felix read it as we should only log a warning if we saw a config option that we didn't support anywhere in the agent and that we should try to apply all config options we got from the APM Server - beyond transaction_sample_rate.
The reason this is easy for the Java agent is that they already have support for what they call "dynamic config options" which are all documented. In the Node.js agent, while there's a lot of config options you probably could update on the fly, this has not been tested and the agent might behave unexpectedly if you try to update any config option other than transaction_sample_rate. In the Node.js agent, we could go through the process of documenting and testing which could be dynamically updated and which couldn't - we just haven't yet as we didn't think it was part of this issue.
Status: Undecided
Felix main reason to want the default to be "enabled" is:
My main reason to want the default to be "disabled" is:
/config/v1/agents while it's disabled, which currently will lead to errors being logged in the Node.js agent.We agreed that it was unfortunate that the APM Server responds with a 500 if kibana.enabled is false (which will be the default value for new deployments), but this is probably too late to change (right @simitt?).
The Node.js agent treats a 500 response as something that should be logged at an error level, and while technically we could treat it differently by feature-detecting it using the Cache-Control header, this would be a very brittle fix that would easily break in the future.
We could maybe decide to only log this error the first time, but this would again add state and a bunch of follow up questions (e.g. what if you get 10x 500 and then a 200 and then another 500, should it then log the error once more?). So all this just seem like hacks to get around the 500 error logging issue in the first place, which would be easily avoided by defaulting to having this feature disabled.
Status: Undecided
We both agreed this would be a good idea as some of the confusion comes from trying to read this and other issues to piece together the most up to date information.
Status: 👍👍
@watson @felixbarny we discussed that the 500 is not the best status code to return, so by now the APM Server returns a 403 if Kibana is disabled, and a 503 if Kibana is unreachable. There is no expected case in which a 500 is returned.
See https://github.com/elastic/apm-server/issues/2383. This will be backported to 7.3 as a bugfix.
Regarding the config option, we had a discussion elsewhere where it seems like people agree to call it centralConfig (as opposed to remoteConfig etc). Unless there are any objections here, we'll settle on that.
Currently, our validator just gives a boolean result. But I guess we could refactor it to give an error message instead. But I didn't consider this super important as the rules are all documented. Is this important you think?
I think it's important, but not more important than getting the feature in in its basic form. In the case of central config, it _shouldn't_ really be an issue if the agents and UI are aligned on validation rules. This is just something I always do in validation. I personally appreciate having the reason given straight away, as opposed to having to go and find the relevant docs.
My main reason to want the default to be "disabled" is:
- It's disabled in the APM Server by default
By this do you refer to the fact that apm-server.kibana.enabled is false by default?
That makes sense as a generic default because the server would not generally be installed on the same host as Kibana, but won't necessarily be the case in all environments, e.g. if the ECK operator were updated to configure APM Server to talk to Kibana it would be pretty straightforward to configure this, so I could easily see it being more common for it to be enabled than not in those cases.
@simitt will the server/Kibana connection be enabled by default in Cloud?
Document spec in agent-development.md
We both agreed this would be a good idea as some of the confusion comes from trying to read this and other issues to piece together the most up to date information.
:+1: I've created https://github.com/elastic/apm/pull/114. Let's take the remainder of the debate there.
@axw the current plan is to enable Kibana config by default on Cloud.
Update: It looks like the Kibana index has changed from .apm-cm to .apm-agent-configuration. This means if you manually add configurations via Kibana dev console, please ensure to update your index, otherwise you will receive 404s. @bmorelli25 might be interesting for you, I think I saw you reporting this somewhere.
By this do you refer to the fact that
apm-server.kibana.enabledisfalseby default?
Yes.
Btw, now that we changed the APM Server to not return 500 in case it's disabled, but instead 403, my concern is basically mitigated. So I'm ok with having it on by default now.
In case the user is running an older version of the APM Server they'll get a 404, which the Node.js agent will just silently ignore in this case as it's treated the same as if there's no config available for the agent.
Only in the case of 503, which is given if the APM Server is configered with kibana.enabled: true, but can't reach Kibana, then we'll log an "error" level message in the agent. But that's ok I think.
How should the agent behave in the situation where it is starting but the remote configuration has not been applied yet?
A few options:
Another question: What if the APM Server returns with an error or is offline?
ELASTIC_APM_TRANSACTION_SAMPLE_RATE=0 env variable and set transaction_sample_rate=true in the remote config.My vote goes for last bullet points of both questions but that requires good docs (cc @bmorelli25)
How should the agent behave in the situation where it is starting but the remote configuration has not been applied yet?
I think the optimal solution would be to "Disable the agent until there's a response from the APM Server", but this might not be easy to implement for all agents. So while inferior, I've opted to implement the 3rd option in the Node.js agent for now. I don't think we can implement the optimal solution before 7.3.
Another question: What if the APM Server returns with an error or is offline?
Disabling the agent as mention in the first bullet would probably be problematic as it will then never become active again even after the APM Server comes online 🤔
I agree good docs is probably the way to go for now 👍
Documenting that is definitely a good idea. Might also make sense to add some of this to the Kibana UI. There's a delete warning, but we could probably add some of this info on the flyout before adding a config.
Disabling the agent as mention in the first bullet would probably be problematic as it will then never become active again even after the APM Server comes online 🤔
@watson If by "Disabling" we will do as if recording=false (recording from https://github.com/elastic/apm/issues/92) then it should work, shouldn't it?
It would also make sense for agent to be non-recording state since if APM Server returns with an error or is offline then agent won't be able to send it the collected data anyway so what is the point of collecting it?
@watson If by "Disabling" we will do as if recording=false (recording from #92) then it should work, shouldn't it?
That's what I meant by disabling - apply the instrumentations but turn the agent into noop mode.
It would also make sense for agent to be non-recording state since if APM Server returns with an error or is offline then agent won't be able to send it the collected data anyway so what is the point of collecting it?
That's a good point! The only counter-argument would be that the agents only retry after 5 min in case the APM Server is not available. That would yield in a sub-optimal getting-started experience for someone who starts the APM Server shortly after starting the agent-monitored application.
Maybe retry strategy needs to be a little bit different shortly after agent's startup? Instead of waiting 5 minutes agent can try every 5 seconds during the first minute of its lifetime or something like that...
@SergeyKleyman wrote:
@watson If by "Disabling" we will do as if recording=false (recording from #92) then it should work, shouldn't it?
Good point. I was thinking of buffering. Disabling is easier to do of course 👍
However, unless we decide it's a blocker, I'd prefer to keep my current implementation for the 7.3 release.
The question is whether we need to align on this in the first iteration (is it marked as beta?) and what it means in terms of documentation and consistency. If feasible, we could have the spec somewhat flexible and define that agents SHOULD disable themselves but MAY fall back to the other configuration sources if the APM Server does not respond with 200.
I think we should be consistent with this in the first release, and do the simplest thing: start the agent as we do today, with locally-defined config, and update when a positive response is received.
If we later go with disabling until first positive response, I suggest we align again to avoid confusing users of multiple agents.
Trying to put it all together (blocking/non-blocking, active/non-active), can we agree on this:
Current implementation:
Future implementation:
active/recording to falseMost agents don't do healthchecks. A failing healthcheck should also not lead to the agent not polling for the configuration as the APM Server might be temporarily down when the agent starts.
The Java agent only performs a health check on startup so it can write to the logs whether the APM Server is available (also logs version number) on startup with is useful for debugging purposes.
What would be relatively easy to do in the Java agent is to initialize the ApmServerConfigurationSource with active=false. As soon as the configuration is read from remote, that setting would be overridden. If the Server responds with a 404, the settings for this configuration source would be overridden with an empty map. I'm not sure how feasible that is for other agents as they might not work with this configuration source concept where each config source has a map of config keys to values. As ApmServerConfigurationSource has the highest precedence, initially setting active=false there would temporarily disable the agent.
The caveat of this is
The only counter-argument would be that the agents only retry after 5 min in case the APM Server is not available. That would yield in a sub-optimal getting-started experience for someone who starts the APM Server shortly after starting the agent-monitored application.
@felixbarny As I mentioned above you can mitigate "only retry after 5 min" by polling much more frequently during the first X minutes after agent startup.
Most agents don't do healthchecks.
Then where applies
A failing healthcheck should also not lead to the agent not polling for the configuration as the APM Server might be temporarily down when the agent starts.
Right, it shouldn't. The current implementation in the Java agent doesn't fit well anymore with polling. If the purpose of healthcheck is to know whether an APM server is available, it makes no sense to fail and start polling some other endpoint. What we can do is remove the healthcheck and rely only on the remote config for that, or switch to remote config polling only after successful healthcheck
@eyalkoren
it makes no sense to fail and start polling some other endpoint.
Why? I assume by "other endpoint" you refer to more than one URL in "server-URLs" configuration, right? If so why doesn't it make sense for agent to try and switch to the next URL and try to communicate with it instead (get configuration, send data ,etc.)?
@SergeyKleyman no, I meant if you fail to get a proper response for the healthcheck (localhost:9200/), it doesn't make sense to start polling a different path. It is not about rotating between different servers.
I'll try to rephrase:
You can have two states- connected and not_connected. The initial state is not_connected.
In this state, you rotate over your server URLs until you get a valid connection (using healthcheck), in which case you switch to connected and start a polling cycle for remote config (and become active). Whenever the state changes back to not_connected (because remote config fails or sending events fails) - you stop remote config polling (and become inactive) and go back to your not_connected state, looking for a connection.
I feel like introducing state management overcomplicates things and optimizes for an edge case. In the end, we still want to poll for remote config even if the APM Server is currently unavailable. Whether to try to get the remote config right after a failed healthcheck or to wait for a while does not seem like a significant enough improvement to warrant the complexity.
Again, the healthcheck in the Java agent is only executed once on startup for the purposes of having a log line which contains the APM Server version and the status of the connection which helps with debugging/support. It's a completely different concern and should therefore not influence remote configuration polling.
I feel like introducing state management overcomplicates things and optimizes for an edge case.
I definitely not think state management overcomplicates it and not sure why you refer to it as edge cases. Connection state seems the most reasonable way for me to maintain communication with one of unknown number of servers, where being required to switch shouldn't be considered as edge case.
The healthcheck is not important here, anything that you can poll on would fit. The problem with the new remote config resource is that you cannot rely on it because older servers don't have it. Otherwise, it would be enough for this purpose as well.
Another case that just came up on my side while testing: how do we handle deleted configs?
Scenario:
What do we do now: Revert to local configuration value? Revert to default? Do nothing? Everything but "do nothing" would require somewhat major changes in my implementation, as we'd need to keep track of which options have been changed via remote config, and what their local/default value was.
/edit: we discussed this in the agents meeting of July 10. Felix explained that his configuration system has multiple configuration sources with a precedence hierarchy, so removing the remote config source (which has the highest precedence) automatically reverts to the "local" source with the highest precedence.
We agreed that this is a good guideline implementation for other agents to follow, but that it is not feasible to implement this for 7.4 for most agents. As such, agents should document known limitations of their own implementations in the docs, and update those docs once the limitations have been fixed.
Regarding the treatment of default 300s max-age for errors: I'm beginning to think that the server should respond with the same max-age for 404s as for 200s. At the moment the agent will go to sleep for 5 minutes before checking again if there's _new_ config, but once it's got config it'll start querying every 30 seconds. Is there a reason not to make them the same?
@axw Maybe we should use Null object pattern and instead of using a special status code (404) server can return the empty configuration?
@axw the reason is we agreed on returning 5mins for all error cases. We could change to 30s again for non-server side errors. (But we should make this decision soon).
I'm +1 for changing the interval to 30s in case of 404s. That makes it much quicker to apply the config when creating a new config for a particular service.
The downside is that agents will poll quite frequently even if the user never even intends to use central config. But I don't think this will cause much overhead or problems.
The downside is that agents will poll quite frequently even if the user never even intends to use central config. But I don't think this will cause much overhead or problems.
In such cases I'd expect ACM to be turned off in the server, or the cache expiration time to be increased.
the reason is we agreed on returning 5mins for all error cases.
@simit Yeah and I agree with that, however after starting implementation it occurred to me that this isn't really an error case at all. Agents always have to ask for config, and the fact that there isn't any is an expected/normal outcome.
We could change to 30s again for non-server side errors. (But we should make this decision soon).
I think we should change to 30s for 404 specifically, but not for all client errors (not 403.)
Maybe we should use Null object pattern and instead of using a special status code (404) server can return the empty configuration?
@SergeyKleyman I'd be fine with this too. I'm not sure there's much if any value in the 404 status code, since agents essentially need to treat it the same as a 200.
So what exactly is the consensus for the mapping of status codes to polling interval? 404 is 30s and everything else is 5m?
It's always the Cache-Control: max-age=<value in sec> value unless that is not present or valid. In that case, the default is 5 min. The only case when the APM Server will return max-age=300 (5min) is when there is an internal server error. In other cases, it returns max-age=30 (30s)
@elastic/apm-agent-devs also, in case you've not seen the apm-server PR linked above, the server will now respond with status 200 OK.
IIANM, the response body in this case is currently empty. I think it should be {}, so agents don't need to special-case this type of response. I think the server should also return an Etag in the empty case, so that the agents can next time send it and get back a 304 indicating that there's been no change.
I also kept updating the initial description of this Issue, last edit see section Update for 7.3 so the agreements are summarized (as this Issue thread has grown quite big).
@felixbarny this is not entirely true, as there could also be a 400 or 405 in which cases server also returns max-age=300. The server only returns max-age=30 for succesful responses.
For a 403 there is no Etag, as the request is already terminated with a killswitch checking that the feature is not enabled.
@bmorelli25 mentioned that we refer to this feature differently in the UI, docs, and configs:
APM Agent Configuration (BETA)Central ConfigurationAPM Agent configuration in KibanaSince APM Agent Configuration isn't specific enough in the agent's context, I propose we sync server with the agents, changing server to APM Agent Central Configuration, and leave the UI as is. If we find users are confused during the beta period, we could add Central in the UI as well. Thoughts, particularly @alvarolobato @tbragin?
@graphaelli sounds good to me.
Most helpful comment
To get a working dev environment where I could test the agent against the stack, I did the following (with the help of @simitt) - hopefully this can be helpful for others who start adding this feature:
master). From within the apm-integration-testing repo root, run the following:./scripts/compose.py start master --no-apm-servermasterof the apm-server repo and build it (make+make update). I need to prefix themakecommands withCC=gccasgccisn't my default compiler.apm-server.ymlfile settingkibana.enabled: true(uncomment thekibana:line + inside that group, uncomment theenabled:line, changing its default value totrue)../apm-server -c apm-server.yml -e -d "*"Open the Kibana dev console and run the following commands to add test data (or be creative) [source]:
Run a curl command against the APM Server to test everything is working (I used
opbeans-fortranbecause this name was added in the test data above):Try to add the
If-None-Matchheader with the value of theEtagto see that this also works:Update 7/7: I updated the 5 document examples above, to use the agreed upon
transaction_sample_ratename instead ofsampling_rate.