influxdata / telegraf

Agent for collecting, processing, aggregating, and writing metrics, logs, and other arbitrary data.
https://influxdata.com/telegraf
MIT License
14.51k stars 5.56k forks source link

postgresql_extensible: include queries from the file #13770

Open redbaron opened 1 year ago

redbaron commented 1 year ago

Use Case

When configuring multiple instances of postgresql_extensible plugin with same set of queries for each, queries end up taking majority of space of the resulting config.toml. When Telegraf config comes from Kubernetes ConfigMap there is a hard limit on the object size (1MB-3MB, depending on the setup), which is very easy to reach because of queries repetition.

Adding queries_file param which expects TOML file as if it was specified inline will help not reaching size limits on the config file.

Expected behavior

When given following config:

[[inputs.postgres_extensible]]
address = "..."
queries_file = "queries.toml"

with queries.toml content:

[[inputs.postgres_extensible.query]]
sqlquery = "SELECT 1"

[[inputs.postgres_extensible.query]]
sqlquery = "SELECT 2"

Should have same result as specifying all inline like following:

[[inputs.postgres_extensible]]
address = "..."

[[inputs.postgres_extensible.query]]
sqlquery = "SELECT 1"

[[inputs.postgres_extensible.query]]
sqlquery = "SELECT 2"

Actual behavior

All queries must be specified inline and therefore duplicated for each plugin instance

Additional info

It might be use-case to have generic include_config option available when specifying configuration for any plugin or maybe even anywhere in the whole TOML file, uncluding other sections like [agent] and [global_tags]

powersj commented 1 year ago

Hi,

(1MB-3MB, depending on the setup), which is very easy to reach because of queries repetition.

How many or how large are you queries that you hit that limit? :) That seems surprising you would hit it

Before we go down this road, what about splitting up your config across different files? This would require a second plugin defined, but is that an option?

It might be use-case to have generic include_config option available when specifying configuration for any plugin or maybe even anywhere in the whole TOML file, uncluding other sections like [agent] and [global_tags]

Rather than requiring a single file, telegraf's solution was to allow additional TOML configuration files, which also get the additional features like environment variable substitution and TOML validation. This already has its own problems, but provides the user with a nice way to break things up.

redbaron commented 1 year ago

How many or how large are you queries that you hit that limit? :) That seems surprising you would hit it

current queries.toml is 20KB, that is 50 servers to reach the limit.

Before we go down this road, what about splitting up your config across different files? This would require a second plugin defined, but is that an option?

We already define dozens of plugin instances each running same set of queries. Even if ConfigMap limit wasn't the problem, it is just easier to handle queries config on a side and it keeps it DRY.

As ConfigMap limit is per object overall, not per field, splitting will require creating multiple configmaps with unknown number of them. Each ConfigMap have to be mapped "statically" into Kubernetes Pod so as their numbers change new component has to exist to monitor their count and update Kubernetes Pod spec to add/remove them. With a single ConfigMap it is not necessary.

So overall I reasoned it is more practical to allow config inclusion and change wasn't that complicated.

Rather than requiring a single file, telegraf's solution was to allow additional TOML configuration files, which also get the additional features like environment variable substitution and TOML validation.

When checking if TOML allows native includes I stumbled upon this: https://github.com/toml-lang/toml/issues/397#issuecomment-741623484 . If TOML config is templated, then it is not a big deal to generate endless file with a lot of duplication, but I figured some users are creating config more or less by hand and that's where include directive can be handy.

srebhan commented 1 year ago

@redbaron how about an alternative solution where you extend make the address field to take multiple DSNs (e.g. name it addresses and then group the servers by query-set? Wouldn't this also solve your problem?

redbaron commented 1 year ago

It will be less ergonomic. Currently we report any errors nicely both in logs and in metrics because we have plugin instance per PostgreSQL server. If we batch them all together into single plugin it will be harder to spot problems.

redbaron commented 1 year ago

@powersj , in the light of your comment in #13772 how do you propose to solve this size limitation problem?

redbaron commented 1 year ago

How about new source of the config? in addition to file, directory and HTTP add Kubernetes ConfigMap/Secret? Either spcified by name or as selector to match variable number of configmaps similar to how it matches multiple files in the directory.

As a bonus because Kubernetes client library allows subscribing to changes , Telegraf can detect those changes and reload config, something currently HTTP source doesn't allow to do.

Something like

--config kubernetes:///configmap?label-selector=label1=A,label2=B&field-selector=metadata.namespace=x

Will fetch configmaps from namespace x with labels label1=A1 and label2=B. Syntax for label and field selectors will match kubectl https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/ and https://kubernetes.io/docs/concepts/overview/working-with-objects/field-selectors/

Single configmap from external kubernets server can be specified as:

--config kubernetes://10.10.10.10:443/configmap?field-selector=metadata.name=telegraf-conf
redbaron commented 1 year ago

@srebhan , @powersj , what do you think about proposal above?

powersj commented 12 months ago

@redbaron,

First, my apologies for the delay in a response.

@srebhan and I are not the most k8s literate, so I want to ensure we are all on the same page. The original issue here is that the config map that you are using in the pod is limited to a certain size.

With the addition of a configmap telegraf config option, you could then provide multiple configmaps to pull down and use to avoid the size limitation?

Are the config maps hosted via HTTP? or some other protocol?

Thanks!

redbaron commented 12 months ago

@powersj ,

With the addition of a configmap telegraf config option, you could then provide multiple configmaps to pull down and use to avoid the size limitation?

Correct. We define what is called selector (based on labels or fields like name and namespace) to match ConfigMaps or Secrets , those matched configmaps are then assembled into single config not unlike multiple files from directory or multipl --config options

Are the config maps hosted via HTTP? or some other protocol?

Kubernetes API is available over HTTP, but plain HTTP client will miss too many quality of life features offered by official client-go library, namely:

Example use of client-go API can be found in prometheus input plugin, where it lists and then watches pods. When used for config it will be listing and watching ConfigMaps and/or Secret objects instead of Pods, but look and feel will be very similar.

powersj commented 11 months ago

Hey,

Thanks for the additional details. Sven and I are happy to see a PR to add this.

One immediate concern is the base binary size for users doing custom builds, would be nice to understand how much the additional k8s client brings in to a stripped down build.

redbaron commented 11 months ago

It is not small, I was planning to make it optional with build tags