Add ClickHouse input plugin #3894

kshvakov · 2018-03-16T08:15:32Z

Required for all PRs:

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

subuk · 2018-08-23T16:49:21Z

Will this plugin be released?

glinton · 2018-08-23T17:48:41Z

plugins/inputs/clickhouse/clickhouse.go

+)
+
+const sampleConfig = `
+### ClickHouse DSN


Add two spaces before each of these.

glinton · 2018-08-23T18:30:57Z

It looks like it's good to go, apart from the update to dep. Update the branch with our master and fix address this small change.

kshvakov · 2018-08-27T11:02:41Z

Done!

glinton · 2018-09-04T16:11:03Z

@kshvakov You will need to remove the Godeps file and add your dependency with dep.

dep ensure -add github.com/[dependency]/[new-package]

glinton · 2018-09-04T16:11:56Z

Godeps

@@ -0,0 +1,97 @@
+collectd.org 2ce144541b8903101fb8f1483cc0497a68798122


Remove this file and use dep

kshvakov · 2018-09-05T06:37:42Z

@glinton I updated the dependencies.

alkersan · 2018-09-13T14:09:05Z

Hi @glinton! Would be great to have it in 1.8. Is it possible?

captify-daleksandrov · 2018-10-31T06:39:21Z

@glinton @danielnelson Is it possible by any chance to make this into 1.9?

cedwardstesla · 2019-01-17T20:19:29Z

Any chance this can be merged for the 1.10 release?

cedwardstesla · 2019-01-18T20:14:59Z

plugins/inputs/clickhouse/README.md

+```
+# Read metrics from one or many ClickHouse servers
+[[inputs.clickhouse]]
+  dsn     = "native://localhost:9000?username=user&password=qwerty"


Recommend updating this to specify that a database reference is required. I built off of this branch and ran into some issues with getting the plugin to work without specifying a database, but didn't get an obvious error (either silent failure or driver: bad connection). This might be obvious for people who are familiar with Clickhouse but I only maintain the TICK stack so it wasn't immediately obvious to me. I looked up the documentation here to figure out what else may have been needed, ended up just the DB name reference.

I'm not seeing this myself in testing, though I just have a small cluster and a user defined like:

<yandex> <users> <dbn> <password>dbn</password> <profile>default</profile> <quota>default</quota> <networks> <ip>192.168.122.0/24</ip> </networks> </dbn> </users> </yandex>

I do notice I have a default_database set, perhaps it is related?

alyarskiy · 2019-04-16T10:48:28Z

Hi! Any chances to merge this request by the summer?)

kshvakov · 2019-05-16T06:25:59Z

Conflicts resolved

danielnelson

@kshvakov Thanks so much for the contribution and your patience, below is my review.

I do also have one question on the driver code, what is the purpose of this goroutine? We want to ensure that we don't need to devote resources to this plugin when it isn't being used, so we try to keep static initialization and especially static goroutine creation out of the dependencies.

danielnelson · 2019-07-16T06:46:03Z

Gopkg.toml

@@ -224,6 +224,10 @@
  source = "https://github.com/fsnotify/fsnotify/archive/v1.4.7.tar.gz"
  name = "gopkg.in/fsnotify.v1"

+[[constraint]]
+  branch = "master"


Use a version constraint instead of a branch contraint.

danielnelson · 2019-07-16T06:49:49Z

plugins/inputs/clickhouse/README.md

+```
+# Read metrics from one or many ClickHouse servers
+[[inputs.clickhouse]]
+  dsn     = "native://localhost:9000?username=user&password=qwerty"


Is there a difference between using the native:// scheme and tcp://? All documentation I have seen is using the tcp:// form so I suggest we switch over to avoid confusion setting up the DSN.

danielnelson · 2019-07-16T07:35:47Z

plugins/inputs/clickhouse/clickhouse.go

+	{
+		baseQuery.Del("alt_hosts")
+		baseDSN.RawQuery = baseQuery.Encode()
+	}


What is the significance of this scope { block }?

danielnelson · 2019-07-16T07:39:47Z

plugins/inputs/clickhouse/README.md

+[[inputs.clickhouse]]
+  dsn     = "native://localhost:9000?username=user&password=qwerty"
+  cluster = true # If a setting is "true" plugin tries to connect to all servers in the cluster (system.clusters)
+  ignored_clusters = ["test_shard_localhost"] ## ignored cluster names


I'm a little confused by this, but it probably has to do with my limited knowledge of Clickhouse. Can a clickhouse server be part of multiple clusters?

danielnelson · 2019-07-16T07:41:26Z

plugins/inputs/clickhouse/clickhouse.go

+	}
+	conns := make([]*connect, 0, len(ch.clustersConn))
+	for _, conn := range ch.clustersConn {
+		if err := conn.Ping(); err == nil {


I think we should remove the ping, if we can't reach the cluster server I think it should be an error in Gather. You would need to fix network connectivity, ignore the cluster, or switch off of cluster discovery mode.

danielnelson · 2019-07-18T04:09:13Z

plugins/inputs/clickhouse/clickhouse.go

+		if err := rows.Scan(&key, &value); err != nil {
+			return err
+		}
+		fields[key] = value


We should snake_case the field keys, you can use internal.SnakeCase(key).

danielnelson · 2019-07-18T05:17:53Z

plugins/inputs/clickhouse/clickhouse.go

+type ClickHouse struct {
+	DSN             string   `toml:"dsn"`
+	Cluster         bool     `toml:"cluster"`
+	IgnoredClusters []string `toml:"ignored_clusters"`


I'd like to add two options for this: cluster_include and cluster_exclude, then we can build a filter.IncludeExcludeFilter and use it to control which clusters we collect. This just fits in with other plugins a bit more seamlessly.

danielnelson · 2019-07-18T05:21:20Z

plugins/inputs/clickhouse/clickhouse.go

+		return err
+	}
+	for _, conn := range conns {
+		if err := ch.gather(conn, acc); err != nil {


Since each connection here is to a different server, I suggest we run a goroutine for each database and, of course, wait for them with a sync.WaitGroup before returning.

danielnelson · 2019-07-18T05:31:40Z

plugins/inputs/clickhouse/README.md

+```
+# Read metrics from one or many ClickHouse servers
+[[inputs.clickhouse]]
+  dsn     = "native://localhost:9000?username=user&password=qwerty"


I'm not seeing this myself in testing, though I just have a small cluster and a user defined like:

<yandex> <users> <dbn> <password>dbn</password> <profile>default</profile> <quota>default</quota> <networks> <ip>192.168.122.0/24</ip> </networks> </dbn> </users> </yandex>

I do notice I have a default_database set, perhaps it is related?

danielnelson · 2019-07-18T06:16:32Z

plugins/inputs/clickhouse/clickhouse.go

+		if len(conn.shardNum) != 0 {
+			tags["shard_num"] = conn.shardNum
+		}
+		acc.AddFields("clickhouse_tables",


My understanding is that this metric is mergetree specific, and not just any table will appear here. With that in mind, perhaps a more descriptive name would be clickhouse_mergetree or clickhouse_parts?

kshvakov · 2019-09-25T06:18:23Z

I opened a new PR #6441 instead of this

kshvakov · 2019-09-25T06:59:46Z

@glinton @danielnelson

kshvakov added 2 commits March 16, 2018 09:49

Add ClickHouse input plugin

9ebb726

fix import path

d0dcf8e

danielnelson added triage new plugin labels Mar 16, 2018

check interface

572b647

kshvakov force-pushed the master branch from 08a797f to 363f82e Compare March 17, 2018 06:10

collect metrics from the cluster

88c0e0f

kshvakov force-pushed the master branch from 363f82e to 88c0e0f Compare March 17, 2018 06:17

set connect limits

610c583

russorat removed the triage label Mar 22, 2018

glinton suggested changes Aug 23, 2018

View reviewed changes

kshvakov added 2 commits August 27, 2018 13:53

Merge remote-tracking branch 'upstream/master'

19bee59

dep: update

d2f5282

glinton approved these changes Sep 4, 2018

View reviewed changes

glinton suggested changes Sep 4, 2018

View reviewed changes

Godeps Outdated

@@ -0,0 +1,97 @@

collectd.org 2ce144541b8903101fb8f1483cc0497a68798122

Copy link

Contributor

glinton Sep 4, 2018

Choose a reason for hiding this comment

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

Remove this file and use dep

vendor up

5947306

glinton approved these changes Sep 5, 2018

View reviewed changes

glinton requested a review from danielnelson September 14, 2018 17:22

danielnelson added this to the 1.10.0 milestone Nov 6, 2018

russorat modified the milestones: 1.10.0, 1.11.0 Jan 14, 2019

cedwardstesla reviewed Jan 18, 2019

View reviewed changes

stefanhorning mentioned this pull request May 13, 2019

MPX Version of telegraf mediapeers/telegraf#3

Closed

kshvakov added 2 commits May 16, 2019 09:18

fix conflicts

caab067

sync:

c71482a

danielnelson modified the milestones: 1.11.0, 1.12.0 May 24, 2019

danielnelson suggested changes Jul 18, 2019

View reviewed changes

danielnelson removed this from the 1.12.0 milestone Aug 5, 2019

kshvakov mentioned this pull request Sep 25, 2019

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

Merged

3 tasks

kshvakov closed this Sep 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ClickHouse input plugin #3894

Add ClickHouse input plugin #3894

kshvakov commented Mar 16, 2018

subuk commented Aug 23, 2018

glinton Aug 23, 2018

glinton commented Aug 23, 2018

kshvakov commented Aug 27, 2018

glinton commented Sep 4, 2018 •

edited

Loading

glinton Sep 4, 2018

kshvakov commented Sep 5, 2018

alkersan commented Sep 13, 2018

captify-daleksandrov commented Oct 31, 2018

cedwardstesla commented Jan 17, 2019

cedwardstesla Jan 18, 2019

danielnelson Jul 18, 2019

alyarskiy commented Apr 16, 2019

kshvakov commented May 16, 2019

danielnelson left a comment

danielnelson Jul 16, 2019

danielnelson Jul 16, 2019

danielnelson Jul 16, 2019

danielnelson Jul 16, 2019

danielnelson Jul 16, 2019

danielnelson Jul 18, 2019

danielnelson Jul 18, 2019

danielnelson Jul 18, 2019

danielnelson Jul 18, 2019

danielnelson Jul 18, 2019

kshvakov commented Sep 25, 2019

kshvakov commented Sep 25, 2019

		@@ -0,0 +1,97 @@
		collectd.org 2ce144541b8903101fb8f1483cc0497a68798122

Add ClickHouse input plugin #3894

Add ClickHouse input plugin #3894

Conversation

kshvakov commented Mar 16, 2018

Required for all PRs:

subuk commented Aug 23, 2018

Choose a reason for hiding this comment

glinton commented Aug 23, 2018

kshvakov commented Aug 27, 2018

glinton commented Sep 4, 2018 • edited Loading

Choose a reason for hiding this comment

kshvakov commented Sep 5, 2018

alkersan commented Sep 13, 2018

captify-daleksandrov commented Oct 31, 2018

cedwardstesla commented Jan 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyarskiy commented Apr 16, 2019

kshvakov commented May 16, 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

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

kshvakov commented Sep 25, 2019

kshvakov commented Sep 25, 2019

glinton commented Sep 4, 2018 •

edited

Loading