Conversation
plugins/outputs/sql/sql.go
Outdated
| quoted = append(quoted, quoteIdent(column)) | ||
| } | ||
|
|
||
| sql := fmt.Sprintf("INSERT INTO %s(%s) VALUES(%s)", quoteIdent(tablename), strings.Join(quoted, ","), strings.Join(placeholder, ",")) |
There was a problem hiding this comment.
column names might need to be escaped for some weirder column names.
There was a problem hiding this comment.
Yes, there's also opportunity for a sql injection attack through the names of metrics, fields, and tags currently. I'll have to escape them to get them to work and sanitize them so they can't be misused.
There was a problem hiding this comment.
I added identifier quoting and a check for invalid characters that should cover this.
7a021a8 to
268ea71
Compare
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
|
Is there any change to get this in? |
Hi @illuusio, I'd like to complete the to-do list in the PR description and add escaping like ssoroka mentioned in review. I also want to add to the readme, probably reusing some of this PR's description and adding a couple config examples. I hope to get these things done in the next few days and have the PR reviewed and merged in time for the 1.19.0 release candidate on June 2. |
|
@reimda Should I close my PR to favor this? If everything goes like this I'll be happy camper with 1.19. |
|
@illuusio This PR is your PR with tests and a couple fixes added, so it's safe to close your PR. The tests and fixes will make it easier to pass code review and hopefully a little easier to use. I'm glad to hear you're happy about the changes I've made and the plan to get it into 1.19. I should have contacted you before updating your PR, but I was in a hurry because I have a need for this too. Thanks for your work writing #4205! |
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
e676e32 to
5197a05
Compare
… a struct instead of a vector of struct. Remove TagTableSuffix field as it's not used. Switch from metric.Tags and metric.Fields to TagList and FieldList to preserve order of fields and tags so they'll be predictable in unit tests. Don't drop the metric timestamp. Reformat README.md to pass markdown linter.
…to cross compile to other operating systems
…quote strings, double quotes identifiers). Add a test metric with spaces in names. Add a setting for initialization sql that runs right after connection to the db. Use the initialization sql in the mysql integration test to set ANSI_QUOTES mode.
5197a05 to
32b7e88
Compare
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
Add a SQL Output plugin that supports multiple DBMSes. This PR is an update of #4205 by @illuusio, adding integration tests and the ability to store the metric timestamp.
This plugin uses a simple, hard-coded database schema. The table name is the metric name. There is a column per field and a column per tag. It writes a row for every input metric. This means multiple metrics are never merged into a single row, even if they have the same metric name, tags, and timestamp.
This plugin generates simple SQL that is likely to work on any DBMS. It doesn't use any feature that is specific to a particular DBMS. If you want to use a feature that is specific to a particular DBMS, you may be able to set it up manually outside of this plugin or you may need to use a different plugin altogether. This means this plugin doesn't intend to remove the need for other SQL output plugins that are specialized for a single DBMS, for example #8651 for Postgres.
There is basic support to automatically create tables. Automatic table creation assumes input metrics always have the same tags and fields, and that the type of each field never changes. (The plugin doesn't attempt to alter existing tables to add or change columns based on input metrics). Use cases that don't have metrics with static schemas may require the user to create tables manually or to use a different plugin.
The integration tests run each DBMS in a docker container. Since these tests are relatively slow they are skipped in go's short mode unit tests. This is to allow short mode to complete quickly so it can be run as part of in CI builds for PRs. I intend to add a new nightly CI job that runs non short mode tests so they will be exercised regularly.
Draft PR to do list: