CrateDB output plugin #3210

felixge · 2017-09-08T14:01:32Z

Hello,

this PR implements an output plugin for CrateDB using their Postgres Wire Protocol.

Please let me know what kind of changes, if any, you'd like me to make in order to get this merged.

Many thanks in advance 😊
Felix

Required for all PRs:

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

Not really needed here, but I want circleci to be happy :) https://circleci.com/gh/influxdata/telegraf/8149?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

See https://circleci.com/gh/influxdata/telegraf/8150?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Allows connecting without setting up auth stuff. Probably also a better idea for this in general.

Otherwise it passes on my machine, but not on circle-ci. See https://circleci.com/gh/influxdata/telegraf/8152?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

danielnelson · 2017-09-08T18:41:53Z

Thanks for the pull request @felixge, I'm pretty far behind on reviews, so it might take me a little while before I am able to review. One thing I notice though is that the plugin uses a different postgres library than the one we use in existing plugins, is it possible to switch to pgx?

felixge · 2017-09-09T09:34:32Z

@danielnelson good idea, I didn't even notice pgx was used already, as I was just looking at other output plugins.

That being said, unfortunately using pgx causes my tests to fail with the following error:

--- FAIL: TestConnectAndWrite (0.02s)
        Error Trace:    cratedb_test.go:30
	Error:      	Received unexpected error:
	            	ERROR: TableUnknownException: Table 'test.pg_type' unknown (SQLSTATE XX000)

This is because pgx queries the postgres catalog when connecting, and CrateDB doesn't implement those catalog tables. As far as I can tell this feature cannot be disabled in pgx.

So if possible, I'd like to keep using lib/pq for this plugin. Let me know what you think.

Cheers
Felix

danielnelson · 2017-09-14T21:39:24Z

Thanks for opening the upstream issue. Lets try to get it resolved in pgx so we can avoid increasing the size of the Telegraf binary. In the meantime it makes sense to leave your pull request with pq in case anyone would like to do their own testing.

felixge · 2017-10-25T09:37:16Z

@danielnelson yes, this is related to prepared statements. Prepared statements should always be used when trying to safely inject data into a SQL query. Unfortunately CrateDB doesn't support a certain variant of the Extended Query format (which is used for prepared statements):

Note: A parameter data type can be left unspecified by setting it to zero, or by making the array of parameter type OIDs shorter than the number of parameter symbols ($n) used in the query string.

CrateDB however doesn't determine the number of parameters by looking at the number of placeholders, but instead looks at the length of the array of parameter type OIDs. Pgx (and lib/pq) are not setting this array (as it's optional), causing CrateDB to always complain about too many parameters being given.

Of course one can always try to inject the data by manually escaping it on the client site (which is what sanitize.SanitizeSQL and also my code in this PR) does, but this is always prone to error. But in case of CrateDB, there is no choice.

The only question is: Would you prefer to use the escaping/sanitize mechanism from pgx over the code in this PR? If yes, I'll try to change my code accordingly. But it will most likely require giving up on using the database/sql interface, because the sanitize pkg of pgx is internal, so I can only use it indirectly by using the pgx native APIs ...

Please let me know how you'd like to proceed.

I agree about the object literals. I've added good test coverage for that code to make sure it works.

danielnelson · 2017-10-26T22:16:00Z

I guess using sanitize is not an option due to being internal, maybe in the future after pgx gains the ability to not use prepared statements we can update it.

felixge · 2017-10-27T16:07:14Z

@danielegozzi yeah. I also think CrateDB will fix the problem preventing prepared statement support on their end at some point. That will be even better.

Anyway, do you think we can move forward with this PR? If yes, are there any remaining improvements you'd like me to make?

danielnelson · 2017-10-27T21:40:55Z

I'll try to do a closer review soon, shouldn't be a problem to finish this for 1.5.

danielnelson · 2017-11-01T23:25:32Z

plugins/outputs/cratedb/cratedb.go

+		// -> ERROR:  SQLParseException: For input string: "14305102049502225714"
+
+		cols := []interface{}{
+			int64(m.HashID()),


I hadn't considered the values of this hash to be part stable across versions, would it be a problem if this hash changed even if the series key (measurement name + tagset) did not?

Np. I think a cryptographic hash function makes more sense for the use case (deduplication) anyway. Let me know if you think 626f653 works for you.

danielnelson · 2017-11-01T23:35:54Z

plugins/outputs/cratedb/cratedb.go

+
+		cols := []interface{}{
+			int64(m.HashID()),
+			m.Time().In(loc),


Why in local time, what about just sending all times in UTC?

CrateDB converts to UTC anyway, but you're right, this actually makes things a bit simpler. e246032

danielnelson · 2017-11-01T23:46:32Z

plugins/outputs/cratedb/cratedb_test.go

+		{map[string]interface{}{}, `{}`},
+		{map[string]interface{}(nil), `{}`},
+		{map[string]interface{}{"foo": "bar"}, `{"foo" = 'bar'}`},
+		{map[string]interface{}{"foo": "bar", "one": "more"}, `{"foo" = 'bar', "one" = 'more'}`},


Can you add some extra test cases for string escaping, here are some ideas:

map[string]interface{}{"fo"o": "b'ar", "ab'c": `xy"z`, `on"""e`: "mo'''re"}

I've added the additional test case in c434322 and a new smoke test that validates that the escaped value is a valid SQL literal according to CrateDB: 8b85f0b

danielnelson · 2017-11-01T23:47:29Z

Makefile

@@ -86,6 +86,12 @@ docker-run:
 		-e SLAPD_CONFIG_ROOTPW="secret" \
 		-p "389:389" -p "636:636" \
 		-d cobaugh/openldap-alpine
+	docker run \--name cratedb \


Do we need to remove the backslash here? \--name

danielnelson · 2017-11-01T23:48:03Z

plugins/outputs/cratedb/README.md

+
+## Table Schema
+
+The plugin requires a a table with the following schema.


Extra a in this sentence

danielnelson · 2017-11-01T23:49:51Z

plugins/outputs/cratedb/docker-compose.yaml

@@ -0,0 +1,7 @@
+version: "3"


What do we need the docker compose file for?

I was using it for local testing. Shouldn't have checked it in. It's gone as of a54054f

I just used this for local testing.

CrateDB converts to UTC anyway, and this simplifies the code a bit.

See influxdata#3210 (comment)

felixge · 2017-11-02T20:03:22Z

@danielnelson Thanks for taking the time to review this PR. I've addressed all concerns you raised, please let me know if you'd like me to make any additional changes.

For some reason some circleci tests failed:

--- FAIL: TestWriteHTTPSNoClientAuth (0.01s)
	Error Trace:	http_listener_test.go:233
	Error:      	Received unexpected error:
	            	Post https://localhost:33545/write?db=mydb: x509: certificate has expired or is not yet valid
2017/11/02 19:58:12 I! Started HTTP listener service on :0
2017/11/02 19:58:12 http: TLS handshake error from 127.0.0.1:36421: remote error: tls: bad certificate
2017/11/02 19:58:12 I! Stopped HTTP listener service on  :0
--- FAIL: TestWriteHTTPSWithClientAuth (0.02s)
	Error Trace:	http_listener_test.go:247
	Error:      	Received unexpected error:
	            	Post https://localhost:42092/write?db=mydb: x509: certificate has expired or is not yet valid
...
FAIL
FAIL	github.com/influxdata/telegraf/plugins/inputs/http_listener	6.757s

But I think those are unrelated to my changes.

felixge · 2017-11-02T20:17:38Z

I just merged master into this branch, and there is now an additional test failure:

--- FAIL: TestRateLimiter (0.25s)
	<autogenerated>:1: 
                          
	Error Trace:	limiter_test.go:26
		
	Error:      	Not equal: 
		
	            	expected: 5
		
	            	received: 0
FAIL
FAIL	github.com/influxdata/telegraf/internal/limiter	0.523s

That being said, the latest commit on master is also failing CircleCI right now, so I still think those failures are unrelated to this PR.

danielnelson · 2017-11-07T23:36:59Z

I fixed the http_listener issue and it should pass now if you rebase.

The test below failed on the previous run, but it seems unrelated to this PR. --- FAIL: TestRunTimeout (0.10s) <autogenerated>:1: Error Trace: internal_test.go:57 Error: Should be true FAIL FAIL github.com/influxdata/telegraf/internal 0.267s

felixge · 2017-11-09T13:03:00Z

@danielnelson I merged the latest changes from master into this PR and got b0d6723 to pass the circleci checks.

Please let me know if this makes this PR mergeable or if you'd like to see some more improvements.

felixge · 2017-11-10T07:06:44Z

@danielnelson thank you so much for taking the time to review and merge this PR. 🙏

felixge added 21 commits September 8, 2017 15:55

cratedb: basic scaffolding

ea6d517

WIP: mostly worked on escaping stuff

bbb45eb

Get basic INSERT to work! :)

4f46833

Add hash_id field and PRIMARY KEY

4eaa158

smoke test for making sure row was written

6321c16

fix: HashID is uint64 but CrateDB wants int64

e1a97d0

remove comment

91e10af

no need to cancel our contexts

4f72f48

Finish sample config

f4d159f

Whitespace

1393f7e

Initial docs

50a7665

use testutil.GetLocalHost()

3e146c6

docs: wording

ab1a638

add link from main README

83d0981

hash_id INDEX OFF and remove NOT NULL

5ccca8b

Godeps: add lib/pq

6dd8f59

govet: call cancel() func

11b0e39

Not really needed here, but I want circleci to be happy :) https://circleci.com/gh/influxdata/telegraf/8149?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

make: add cratedb for docker tests

dc71427

See https://circleci.com/gh/influxdata/telegraf/8150?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

make: disable crate enterprise edition

71516e9

Allows connecting without setting up auth stuff. Probably also a better idea for this in general.

remove debugging statement

86f7002

fix: test needs fixed time.Location

6b4f4f8

Otherwise it passes on my machine, but not on circle-ci. See https://circleci.com/gh/influxdata/telegraf/8152?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

danielnelson added the triage label Sep 8, 2017

danielnelson added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Sep 8, 2017

felixge mentioned this pull request Sep 9, 2017

Support CrateDB jackc/pgx#320

Closed

Thomas Vogel and others added 3 commits September 27, 2017 17:20

add partitioning by day for crate.io output plugin

90c909d

fix local dev/test setup

a2a651b

Whitespace

7fd0308

danielnelson added new plugin and removed triage labels Oct 27, 2017

danielnelson added this to the 1.5.0 milestone Oct 27, 2017

danielnelson reviewed Nov 1, 2017

View reviewed changes

felixge added 8 commits November 2, 2017 19:44

make: remove backslash typo

3b22f2e

README: remove typo

6c2df9e

remove docker-compose.yaml

a54054f

I just used this for local testing.

Send timestamps in UTC instead of local time

e246032

CrateDB converts to UTC anyway, and this simplifies the code a bit.

escaping add additional test case

c434322

escaping: add smoke test

8b85f0b

Implement new hashID func

626f653

See influxdata#3210 (comment)

remove references to lib/pq

221165d

Merge remote-tracking branch 'origin/master'

efcbbb2

felixge added 3 commits November 9, 2017 13:19

empty commit (trigger ci)

8379cb2

Merge remote-tracking branch 'origin/master'

7a9bfc6

trigger ci

b0d6723

The test below failed on the previous run, but it seems unrelated to this PR. --- FAIL: TestRunTimeout (0.10s) <autogenerated>:1: Error Trace: internal_test.go:57 Error: Should be true FAIL FAIL github.com/influxdata/telegraf/internal 0.267s

danielnelson merged commit 62ec3e5 into influxdata:master Nov 9, 2017

danielnelson mentioned this pull request Nov 10, 2017

Postgresql output plugin (for using with timescaledb) #3408

Closed

danielnelson mentioned this pull request Jan 5, 2018

Convert datid to a string for backwards compatibility #3638

Closed

3 tasks

aromeyer pushed a commit to aromeyer/telegraf that referenced this pull request May 19, 2018

Add CrateDB output plugin (influxdata#3210)

e037500

maxunt pushed a commit that referenced this pull request Jun 26, 2018

Add CrateDB output plugin (#3210)

e1005eb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CrateDB output plugin #3210

CrateDB output plugin #3210

felixge commented Sep 8, 2017

danielnelson commented Sep 8, 2017

felixge commented Sep 9, 2017

danielnelson commented Sep 14, 2017

felixge commented Oct 25, 2017 •

edited

Loading

danielnelson commented Oct 26, 2017

felixge commented Oct 27, 2017

danielnelson commented Oct 27, 2017

danielnelson Nov 1, 2017

felixge Nov 2, 2017

danielnelson Nov 1, 2017

felixge Nov 2, 2017

danielnelson Nov 1, 2017

felixge Nov 2, 2017 •

edited

Loading

danielnelson Nov 1, 2017

felixge Nov 2, 2017

danielnelson Nov 1, 2017

felixge Nov 2, 2017

danielnelson Nov 1, 2017

felixge Nov 2, 2017

felixge commented Nov 2, 2017

felixge commented Nov 2, 2017

danielnelson commented Nov 7, 2017

felixge commented Nov 9, 2017 •

edited

Loading

felixge commented Nov 10, 2017


		## Table Schema

		The plugin requires a a table with the following schema.

CrateDB output plugin #3210

CrateDB output plugin #3210

Conversation

felixge commented Sep 8, 2017

Required for all PRs:

danielnelson commented Sep 8, 2017

felixge commented Sep 9, 2017

danielnelson commented Sep 14, 2017

felixge commented Oct 25, 2017 • edited Loading

danielnelson commented Oct 26, 2017

felixge commented Oct 27, 2017

danielnelson commented Oct 27, 2017

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

felixge Nov 2, 2017 • edited Loading

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

felixge commented Nov 2, 2017

felixge commented Nov 2, 2017

danielnelson commented Nov 7, 2017

felixge commented Nov 9, 2017 • edited Loading

felixge commented Nov 10, 2017

felixge commented Oct 25, 2017 •

edited

Loading

felixge Nov 2, 2017 •

edited

Loading

felixge commented Nov 9, 2017 •

edited

Loading