Add ClickHouse input plugin (instead of #3894) #6441

kshvakov · 2019-09-25T06:13:38Z

Required for all PRs:

Signed CLA.
Associated README.md updated.
Has appropriate unit tests.

This PR replace #3894

danielnelson · 2019-09-26T01:53:20Z

Before I review again, can you comment about the goroutine in the clickhouse library that I mentioned in the top comment of my original review?

kshvakov · 2019-09-26T03:53:14Z

@danielnelson Hi, I didn't use the tcp protocol for the new implementation, I used the HTTP protocol and standard net/http library.
PS: ClickHouse driver uses "cached time" because the real time.Now() is very expensive for it
PS2: clickhouse_tables for metric name, I know that this metric is mergetree specific, but ClickHouse didn't provide another metrics for the tables. If CH will provide new metrics, I'll add them

AlexKomrakov · 2019-10-02T11:57:01Z

Have an error while using this plugin branch:

2019-10-02T11:45:00Z E! [inputs.clickhouse] Error in plugin: strconv.Atoi: parsing "18446743937525109187": value out of range

Didn't find this number in metrics tables. Mayby it can be cause of negative numbers in system.metrics table
Upd: filter all negative values in queries, error still exists

kshvakov · 2019-10-02T12:47:19Z

@AlexKomrakov fixed

danielnelson

Looks good, I think the choice to rewrite against the HTTP API was a good one. I have quite a few comments but it's mostly style and minor issues. The largest issue to resolve is more comprehensive tests.

danielnelson · 2019-10-21T22:34:13Z

plugins/inputs/clickhouse/clickhouse.go

+var defaultTimeout = 5 * time.Second
+
+var sampleConfig = `
+[[inputs.clickhouse]]


In order to get the right generation with telegraf --usage clickhouse, we need to do do a couple clean up tasks:

remove the table(section) header

remove the file output section

add comment for each item, and a newline between each option

don't align the value column

make sure to show the default value, examples can be shown like:
## Static routing key. Used when no routing_tag is set or as a fallback ## when the tag specified in routing tag is not found. ## example: routing_key = "telegraf" # routing_key = ""

try to give defaults for as much as possible, I think we could have them for everything.

to set a value default to true (auto_discovery), set the default in the init function.

More style info: https://github.com/influxdata/telegraf/wiki/SampleConfig

Make sure to mention that cluster_include and cluster_exclude must be exact matches, since it would generally be expected that globs could be used here in keeping with other Telegraf filters.

@danielnelson could you look at this PR again, I improved README and SampleConfig with our style guide recommendations please look at af982f4

plugins/inputs/clickhouse/clickhouse.go

plugins/inputs/clickhouse/clickhouse_test.go

plugins/inputs/clickhouse/clickhouse.go

This reverts commit e53649a.

…/telegraf into feature/plugin-input-clickhouse

…a#6441 (comment) Signed-off-by: Slach <[email protected]>

…plugin-input-clickhouse

Slach · 2020-02-16T07:04:04Z

[x] Signed CLA
@danielnelson could you look to this pull request again?

plugins/inputs/clickhouse/README.md

ssoroka · 2020-02-28T01:47:31Z

plugins/inputs/clickhouse/clickhouse.go

+var sampleConfig = `
+[[inputs.clickhouse]]
+  timeout         = 5
+  servers         = ["http://username:[email protected]:8123"]


does this complicate things if servers have different auth?

ssoroka · 2020-02-28T01:48:45Z

plugins/inputs/clickhouse/clickhouse.go

+	inputs.Add("clickhouse", func() telegraf.Input {
+		return &ClickHouse{
+			AutoDiscovery:      true,
+			InsecureSkipVerify: true,


I'd rather default this to false. Secure by default!

answer to #6441 (comment)
yes, servers can have a different username and password for authorization, so we shall add the ability to set different authorization parameters for different servers, it's no so much complicated

fixed on 353c73b

ssoroka · 2020-02-28T01:50:43Z

plugins/inputs/clickhouse/clickhouse.go

+		Timeout: timeout,
+		Transport: &http.Transport{
+			TLSClientConfig: &tls.Config{
+				InsecureSkipVerify: ch.InsecureSkipVerify,


missing the rest of the config; need to be able to specify certificates.

We should use the standard tls.ClientConfig options, check the http input for an example. In the sample configuration we would add the options:

## Optional TLS Config # tls_ca = "/etc/telegraf/ca.pem" # tls_cert = "/etc/telegraf/cert.pem" # tls_key = "/etc/telegraf/key.pem" ## Use TLS but skip chain & host verification # insecure_skip_verify = false

fixed on 353c73b

plugins/inputs/clickhouse/clickhouse.go

ssoroka · 2020-02-28T02:02:54Z

plugins/inputs/clickhouse/clickhouse.go

+			for _, v := range args {
+				in = append(in, escape(v))
+			}
+			return "cluster " + expr + " (" + strings.Join(in, ", ") + ")"


Suggested change

return "cluster " + expr + " (" + strings.Join(in, ", ") + ")"

return fmt.Sprintf("cluster %s (%s)", expr, strings.Join(in, ", "))

fixed on 353c73b

ssoroka · 2020-02-28T02:03:44Z

plugins/inputs/clickhouse/clickhouse.go

+		return "WHERE " + makeFilter("IN", ch.ClusterInclude)
+	}
+
+	return "WHERE " + makeFilter("NOT IN", ch.ClusterExclude)


it seems like this code would make it so that you can't use both filters at the same time. shouldn't be too hard to adjust to support both

fixed on 353c73b

plugins/inputs/clickhouse/clickhouse.go

danielnelson · 2020-02-28T19:25:57Z

plugins/inputs/clickhouse/clickhouse.go

+var sampleConfig = `
+[[inputs.clickhouse]]
+  timeout         = 5
+  servers         = ["http://username:[email protected]:8123"]


It does require you to define multiple plugins in that case.

danielnelson · 2020-02-28T19:27:51Z

plugins/inputs/clickhouse/clickhouse.go

+	AutoDiscovery      bool     `toml:"auto_discovery"`
+	ClusterInclude     []string `toml:"cluster_include"`
+	ClusterExclude     []string `toml:"cluster_exclude"`
+	Timeout            int      `toml:"timeout"`


For timeout use a internal.Duration so that it can be specified as a string: "5s"

fixed on 353c73b

danielnelson · 2020-02-28T19:37:48Z

plugins/inputs/clickhouse/clickhouse.go

+		Timeout: timeout,
+		Transport: &http.Transport{
+			TLSClientConfig: &tls.Config{
+				InsecureSkipVerify: ch.InsecureSkipVerify,


We should use the standard tls.ClientConfig options, check the http input for an example. In the sample configuration we would add the options:

## Optional TLS Config # tls_ca = "/etc/telegraf/ca.pem" # tls_cert = "/etc/telegraf/cert.pem" # tls_key = "/etc/telegraf/key.pem" ## Use TLS but skip chain & host verification # insecure_skip_verify = false

…plugin-input-clickhouse

influxdata#6441 (comment) influxdata#6441 (comment) influxdata#6441 (comment) influxdata#6441 (comment) influxdata#6441 (comment) influxdata#6441 (comment) influxdata#6441 (comment) influxdata#6441 (comment) Signed-off-by: Slach <[email protected]>

Slach · 2020-03-03T11:55:46Z

Dear @ssoroka and @danielnelson could you look to this PR again?

you could test it with `telegraf --config plugins/inputs/clickhouse/dev/telegraf_ssl.conf` Signed-off-by: Slach <[email protected]>

danielnelson

It's almost perfect, but still a few minor items we need to complete before merging.

danielnelson · 2020-03-03T18:38:31Z

.gitignore

@@ -3,3 +3,4 @@
 /telegraf.exe
 /telegraf.gz
 /vendor
+/.idea


Please revert this, we only add files generated by the build system to it. Instead you can add this to your Git core.excludesfile:

$ cat ~/.gitconfig: [core] excludesfile = /home/dbn/.gitignore $ cat ~/gitignore tags *.pyc *.swp .noseids

sorry, fixed on f7bf396

plugins/inputs/clickhouse/README.md

danielnelson · 2020-03-03T18:55:18Z

plugins/inputs/clickhouse/clickhouse.go

+var sampleConfig = `
+[[inputs.clickhouse]]
+  timeout         = 5
+  servers         = ["http://username:[email protected]:8123"]


This should be done before merging, if the multiple passwords are needed then the plugin can be defined multiple times. On top of consistency with other plugins, it is important to do because this form will cause credentials to be logged, the userinfo section is deprecated, and it is too hard to escape the credentials especially with string passwords.

danielnelson · 2020-03-03T18:57:09Z

plugins/inputs/clickhouse/clickhouse.go

+					c.url = &url.URL{
+						Scheme: u.Scheme,
+						Host:   net.JoinHostPort(c.Hostname, u.Port()),
+						User:   u.User,


Pass credentials using the header method X-ClickHouse-User, X-ClickHouse-Key. This avoids the credentials being logged as part of error messages and anyway the userinfo section of the url is deprecated (rfc 3986).

fixed on f7bf396

plugins/inputs/clickhouse/clickhouse.go

…plugin-input-clickhouse

add user and password options instead of use credentials in servers URL rollback .gitignore influxdata#6441 (comment) influxdata#6441 (comment) influxdata#6441 (comment) Signed-off-by: Slach <[email protected]>

Slach · 2020-03-04T10:46:56Z

@danielnelson could you look this PR last time? I hope current implementation looks good

danielnelson · 2020-03-04T22:48:06Z

I'm making a couple minor tweaks post merge:

rename user option to username
rename hostname tag to source in docs

kshvakov added 2 commits September 25, 2019 09:08

Add ClickHouse input plugin

60f1ea7

go fmt

700e143

kshvakov mentioned this pull request Sep 25, 2019

Add ClickHouse input plugin #3894

Closed

3 tasks

kshvakov added 2 commits September 25, 2019 09:46

fix potential problems with output_format_json_quote_64bit_integers

3f66ac8

add example output

05ebe1a

add chInt64 test

5795522

kshvakov added 2 commits October 2, 2019 15:45

fix parse uint64 value

6058fcb

rm go.mod

07860af

update test

a33f9b2

danielnelson added the new plugin label Oct 7, 2019

danielnelson suggested changes Oct 22, 2019

View reviewed changes

kshvakov added 9 commits December 22, 2019 09:17

sync changes

b8d94cb

fix test

9ceee76

support old Go versions

45060d1

add test

1430995

remove gomod file

e53649a

Revert "remove gomod file"

d04f6c7

This reverts commit e53649a.

remove gomod file

4359baf

minor improvements

4823ec9

make fmt

de92951

sjwang90 added this to the 1.14.0 milestone Jan 13, 2020

Slach added 3 commits January 20, 2020 19:47

Merge branch 'feature/plugin-input-clickhouse' of github.com:kshvakov…

affdbfb

…/telegraf into feature/plugin-input-clickhouse

improve SampleConfig and README.md based on review comments influxdat…

af982f4

…a#6441 (comment) Signed-off-by: Slach <[email protected]>

Merge branch 'master' of github.com:influxdata/telegraf into feature/…

1fc86c8

…plugin-input-clickhouse

sjwang90 requested a review from ssoroka February 24, 2020 19:06

ssoroka suggested changes Feb 28, 2020

View reviewed changes

danielnelson reviewed Feb 28, 2020

View reviewed changes

Slach added 2 commits March 3, 2020 09:35

Merge branch 'master' of github.com:influxdata/telegraf into feature/…

638a2e5

…plugin-input-clickhouse

fix review comments

353c73b

influxdata#6441 (comment) influxdata#6441 (comment) influxdata#6441 (comment) influxdata#6441 (comment) influxdata#6441 (comment) influxdata#6441 (comment) influxdata#6441 (comment) influxdata#6441 (comment) Signed-off-by: Slach <[email protected]>

replace TLS server cert to client cert

db26b0d

you could test it with `telegraf --config plugins/inputs/clickhouse/dev/telegraf_ssl.conf` Signed-off-by: Slach <[email protected]>

ssoroka approved these changes Mar 3, 2020

View reviewed changes

danielnelson reviewed Mar 3, 2020

View reviewed changes

Slach added 2 commits March 4, 2020 13:10

Merge branch 'master' of github.com:influxdata/telegraf into feature/…

0a5d453

…plugin-input-clickhouse

fixes last review comments

f7bf396

add user and password options instead of use credentials in servers URL rollback .gitignore influxdata#6441 (comment) influxdata#6441 (comment) influxdata#6441 (comment) Signed-off-by: Slach <[email protected]>

danielnelson approved these changes Mar 4, 2020

View reviewed changes

danielnelson merged commit e4caa34 into influxdata:master Mar 4, 2020

athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020

Add ClickHouse input plugin (influxdata#6441)

0fc4a3b

veorlo pushed a commit to GlobalNOC/telegraf that referenced this pull request May 4, 2020

Add ClickHouse input plugin (influxdata#6441)

57aceb7

idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020

Add ClickHouse input plugin (influxdata#6441)

f3395e4

arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023

Add ClickHouse input plugin (influxdata#6441)

2e212f1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ClickHouse input plugin (instead of #3894) #6441

Add ClickHouse input plugin (instead of #3894) #6441

kshvakov commented Sep 25, 2019

danielnelson commented Sep 26, 2019

kshvakov commented Sep 26, 2019

AlexKomrakov commented Oct 2, 2019 •

edited

Loading

kshvakov commented Oct 2, 2019

danielnelson left a comment

danielnelson Oct 21, 2019

Slach Feb 16, 2020

Slach commented Feb 16, 2020

ssoroka Feb 28, 2020

ssoroka Feb 28, 2020

Slach Mar 3, 2020

Slach Mar 3, 2020

ssoroka Feb 28, 2020

danielnelson Feb 28, 2020

Slach Mar 3, 2020

ssoroka Feb 28, 2020

Slach Mar 3, 2020

ssoroka Feb 28, 2020

Slach Mar 3, 2020

danielnelson Feb 28, 2020

danielnelson Feb 28, 2020

Slach Mar 3, 2020

danielnelson Feb 28, 2020

Slach commented Mar 3, 2020

danielnelson left a comment

danielnelson Mar 3, 2020

Slach Mar 4, 2020

danielnelson Mar 3, 2020

danielnelson Mar 3, 2020

Slach Mar 4, 2020

Slach commented Mar 4, 2020 •

edited

Loading

danielnelson commented Mar 4, 2020

	return "cluster " + expr + " (" + strings.Join(in, ", ") + ")"
	return fmt.Sprintf("cluster %s (%s)", expr, strings.Join(in, ", "))

Add ClickHouse input plugin (instead of #3894) #6441

Add ClickHouse input plugin (instead of #3894) #6441

Conversation

kshvakov commented Sep 25, 2019

Required for all PRs:

danielnelson commented Sep 26, 2019

kshvakov commented Sep 26, 2019

AlexKomrakov commented Oct 2, 2019 • edited Loading

kshvakov commented Oct 2, 2019

danielnelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Slach commented Feb 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Slach commented Mar 3, 2020

danielnelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Slach commented Mar 4, 2020 • edited Loading

danielnelson commented Mar 4, 2020

AlexKomrakov commented Oct 2, 2019 •

edited

Loading

Slach commented Mar 4, 2020 •

edited

Loading